Date: Tue, 17 Jan 2012 20:58:48 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Pawel Jakub Dawidek <pjd@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Bruce Evans <brde@optusnet.com.au>, Guy Helmer <guy.helmer@palisadesystems.com> Subject: Re: svn commit: r230037 - head/lib/libutil Message-ID: <20120117201411.B1819@besplex.bde.org> In-Reply-To: <20120116175627.GA1674@garage.freebsd.pl> References: <201201122249.q0CMnaZe030200@svn.freebsd.org> <20120114204720.Q1458@besplex.bde.org> <20120114182758.GJ1694@garage.freebsd.pl> <20120115073823.O843@besplex.bde.org> <52A73054-9960-403B-B2FE-857C8801D129@palisadesystems.com> <20120116175627.GA1674@garage.freebsd.pl>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 16 Jan 2012, Pawel Jakub Dawidek wrote:
> On Mon, Jan 16, 2012 at 11:14:32AM -0600, Guy Helmer wrote:
>> I've pasted the diff below that I think captures the majority of the issues you have brought up. I have not attempted to tackle the property.3/properties.3 issues, nor the objections to the prefixes that I think would take considerably more effort to resolve -- I wanted to concentrate on the issues that can be isolated to libutil. I hope the diff was pasted OK, especially WRT <tab> characters.
>
> The patch looks mostly good, one nit mentioned below and also one
> question for Bruce.
>
>> +/* Flags for hexdump(3) */
>> +/* Flags for humanize_number(3) flags */
>> +/* Flags for humanize_number(3) scale */
>> +/* return values from realhostname() */
>> +/* Flags for pw_scan() */
>> +/* Return values from uu_lock() */
>
> All those sentences are missing period and one doesn't start with
> capital letter.
I decided not to worry about the termination since it was fairly consistent
in the file. The most recent commit fixed these, but made all the others
inconsistent:
% /* for properties.c */
% /* Avoid pulling in all the include files for no need */
% #ifdef _STDIO_H_ /* avoid adding new includes */
This is now the only comment to the right of code. Comments to the
right of ifdefs are especially hard format nicely
(the normal comment indentation to 32 or 40 columns doesn't work
well; I normally use a single space; the above indents to 24 columns),
so the should be avoided. The same treatment is used for 3 other includes,
but there is no comment for the others. The simplest fix is to remove
this comment. Otherwise, put a meta-comment about them all somewhere.
It's hard to place this so that it clearly covers them all, but anywhere
is better than to the right of 1 of their ifdefs.
% /* fparseln(3) */
This is also missing the new, otherwise (too) uniform wording
"Flags for..."
> I noticed also one more inconsistency:
>
> struct kinfo_file *
> kinfo_getfile(pid_t _pid, int *_cntp);
> struct passwd
> *pw_dup(const struct passwd *_pw);
>
> Sometimes * is on the same line as function type and sometimes it is in
> the line below. Former is definiately better.
Indded.
> Guy, feel free to commit what you got now with those sentences fixed and
> I'll do one iterration. It is taking way too long and I'm sure you are
> bored by now:) We don't want to scare you off:)
>
>> +struct pidfh *
>> + pidfile_open(const char *_path, mode_t _mode, pid_t *_pidptr);
>
> Bruce, is this your suggestion?
Yes.
> This somehow looks weird too me. What I
> use and I think it is in general more widely used across FreeBSD is
> simply:
>
> struct pidfh *pidfile_open(const char *_path, mode_t _mode, pid_t *_pidptr);
>
> when the type exceeds one tab, but the line fits into 80 chars or:
>
> struct pidfh *pidfile_open(const char *_path, mode_t _mode, pid_t *_pidptr,
> int _some_other_argument);
>
> when line exceeds 80 chars.
Very important headers like <stdlib.h> often use the style of lining
up all function names. This can from 4.4BSD (was even in FreeBSD-1
so probably in Net/2) so I consider it Normal. I think this is easier
to read. But it is harder to write and maintain, so only very important
headers should do it (same for leaving an extra column for '* before
function names'). libutil.h isn't important, but it already did this
in many cases (including by you for humanize_number() :-), so I said
to do it for all cases. A middle way would be to use the fancy
formatting only when line splitting is needed to avoid long lines
anyway:
- short prototypes: never split
- slightly longer prototypes: split at function name provided this requires
only 2 lines
- even longer prototypes: split after a comma in parameter list provided this
requires only 2 lines
- very long prototypes: split after at function name and take more than 3
lines no matter how it is split (might take 1 extra for this)
But this would be even harder to write and maintain.
> Especially this one looks very strange:
>
> properties
> properties_read(int _fd);
>
`properties' is weird because it doesn't look like a type.
indent(1) and maybe smart editors don't even understand the minimal
facy formatting in <stdlib.h>. That's what I mean by "hard to write
and maintain". Especially for the fancier formattings of very long
prototypes which need careful fancy formatting most, it's too hard
to produce and reproduce with a simple editor or formatting utility.
FreeBSD indent(1) still doesn't even understand prototypes, but it
manages to usually not mess up simple ones. It actually makes a
big mess of <stdlib.h>, starting with the first __dead2:
% __BEGIN_DECLS
% void
% abort(void)__dead2;
% int abs(int)__pure2;
% int atexit(void (*) (void));
% double atof(const char *);
After removing the __dead2 and the __pure2, it gets a bit further:
% __BEGIN_DECLS
% void abort(void);
% int abs (int);
% int atexit(void (*) (void));
% double atof(const char *);
% int atoi(const char *);
% long atol(const char *);
This looks OK, but indent has destroyed the fancy 9-char indent (1 extra
for a '*' that is not used in the above). It doesn't understand the
nested declaration of the function pointer in the parameter list for
atexit(), but it has only added a space there.
% void *
% bsearch(const void *, const void *, size_t,
% size_t, int (*) (const void *, const void *));
Here it is more confused by the function pointer...
% void *calloc(size_t, size_t);
% div_t div(int, int)__pure2;
% void exit(int)__dead2;
...so that all subsequent formatting is destroyed (it thinks these
prototypes are local variables or statements).
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120117201411.B1819>