Date: Sun, 25 Feb 2001 23:25:17 +0200 (EET) From: Maxim Sobolev <sobomax@freebsd.org> To: rooneg@electricjellyfish.net (Garrett Rooney) Cc: sobomax@freebsd.org (Maxim Sobolev), ports@freebsd.org, jhk@freebsd.org, gad@freebsd.org, reg@freebsd.org Subject: Re: [patch] which package functionality for pkg_info Message-ID: <200102252125.f1PLPH132145@vic.sabbo.net> In-Reply-To: <20010225161633.C94657@electricjellyfish.net> from "Garrett Rooney" at Feb 25, 2001 04:16:33 PM
next in thread | previous in thread | raw e-mail | index | archive | help
>
> On Sun, Feb 25, 2001 at 10:59:18PM +0200, Maxim Sobolev wrote:
> > >
> > > comments welcome as always.
> >
> > This time only cosmetics, more comprehensive review I'll try to do
> > tomorrow.
>
> thanks a lot!
Don't even mention it! ;)
>
> > > char *CheckPkg = NULL;
> > > +char *CheckFile = NULL;
> >
> > Please be carefull with spaces vs. tabs. Get an editor with space
> > highlighting and reviw all your patch.
>
> oops, i thought i had gotten that one...
>
> > > +/* comparison to see if two paths we're on matches the one we're looking for. */
> >
> > Please break up long lines like the above to the several lines.
>
> done.
>
> > > +static int
> > > +cmp_path(const char *target, const char *current, const char *cwd) {
> >
> > Please read style(9) for the conventions used in FreeBSD code.
> > Particularly, function should be in the form:
> >
> > void
> > foo(int bar)
> > {
> > ...
> > }
>
> ok.
>
> > > + strncpy(temp, cwd, cwd_len);
> > > + strncat(temp, "/", 1);
> > > + strncat(temp, current, current_len);
> >
> > Use strlcpy(3) and sprlcat(3) instead of strncpy/strncat - the former
> > are considerably less error-prone.
>
> cool! i had never seen those before.
BTW, in the example above sprinf(3) or asprintf(3) could be even more
applicable.
> > > + if (!strncmp(target, fixed_path, strlen(target)))
> > > + rval = 1;
> > > + else
> > > + rval = 0;
> >
> > Again, please make indentation consistent with the rest of code.
>
> the way it is now is a 4 space indent, with tabs used wherever there are two
> levels of indent, which is what the rest of the file seems to do. is there
> something else i should be doing?
Identation-wise it should be OK if so. If you haven't yet, pleae read
style(9) manpage to get an idea of how your code should be formatter.
>
> > > + FTS *ftsp;
> > > + FTSENT *entp;
> >
> > Please check if you can leverage yet-to-be added matchinstalled()
> > function (see my recent patches in the freebsd-ports list) to get names
> > of installed packages instead of rolling your own fts(3) loop.
>
> i'll take a look at it.
>
> > > -.Op Fl cdDfgGiIkLmopqrRsvx
> > > +.Op Fl cdDfgGiIkLmopqrRsvWx
> > > .Op Fl e Ar package
> > > .Op Fl l Ar prefix
> > > .Op Fl t Ar template
> > > +.Op Fl W Ar filename
> >
> > You don't have to manifest `W' twice. Please remove it from the
> > `cdDfgGiIkLmopqrRsvWx'.
>
> oops. thanks.
No problem.
-Maxim
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-ports" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200102252125.f1PLPH132145>