From owner-svn-src-head@FreeBSD.ORG Tue Jan 17 09:58:56 2012 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id EC1291065670; Tue, 17 Jan 2012 09:58:56 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail04.syd.optusnet.com.au (mail04.syd.optusnet.com.au [211.29.132.185]) by mx1.freebsd.org (Postfix) with ESMTP id C762E8FC14; Tue, 17 Jan 2012 09:58:55 +0000 (UTC) Received: from c211-30-171-136.carlnfd1.nsw.optusnet.com.au (c211-30-171-136.carlnfd1.nsw.optusnet.com.au [211.30.171.136]) by mail04.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q0H9wmPb028755 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 17 Jan 2012 20:58:49 +1100 Date: Tue, 17 Jan 2012 20:58:48 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Pawel Jakub Dawidek In-Reply-To: <20120116175627.GA1674@garage.freebsd.pl> Message-ID: <20120117201411.B1819@besplex.bde.org> 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> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Bruce Evans , Guy Helmer Subject: Re: svn commit: r230037 - head/lib/libutil X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Jan 2012 09:58:57 -0000 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 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 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 . 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 , 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