Date: Sun, 15 Jan 2012 08:02:30 +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, Guy Helmer <ghelmer@FreeBSD.org>, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r230037 - head/lib/libutil Message-ID: <20120115073823.O843@besplex.bde.org> In-Reply-To: <20120114182758.GJ1694@garage.freebsd.pl> References: <201201122249.q0CMnaZe030200@svn.freebsd.org> <20120114204720.Q1458@besplex.bde.org> <20120114182758.GJ1694@garage.freebsd.pl>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 14 Jan 2012, Pawel Jakub Dawidek wrote: > On Sat, Jan 14, 2012 at 09:59:27PM +1100, Bruce Evans wrote: >> ... >> It's good to declare mode_t, since pidfile_open() uses it and we want >> to remove the dependency on <sys/param.h>. However, this definition >> doesn't follow KNF or the style of all the other typedef declarations >> in the file, since all the others follow KNF and thus have a space >> instead of a tab after #define and also after typedef. > > I think you mixed space with tab. All the others have a tab after > #define and typedef. I fully agree this should be consistent. Oops. >>> -#ifdef _SYS_PARAM_H_ >>> int pidfile_close(struct pidfh *_pfh); >>> int pidfile_fileno(const struct pidfh *_pfh); >>> struct pidfh * >>> pidfile_open(const char *_path, mode_t _mode, pid_t *_pidptr); >>> int pidfile_remove(struct pidfh *_pfh); >>> int pidfile_write(struct pidfh *_pfh); >>> -#endif >> >> Now these are unsorted, since a separate section to hold them is not >> needed. It was used just to make the ifdef easier to read (we don't >> want to split up the main list with blank lines around each ifdef, and >> without such blank lines the ifdefs are harder to read). > > I'd prefer not to change that. All those functions are part of pidfile(3) > API and it would be better, IMHO, to keep them together here too. The functions have a unique prefix, so they are grouped nicely when sorted into a long list. While I'm here, I'll complain about the verboseness of that prefix :-). Other APIs in the file mostly use short prefixes: - kinfo_. Should have been ki_ like its struct member names. pidfile uses a good prefix for its struct member names too. - properties_/property_. Bad, like the rest of the API. - uu_. A weird nondescriptive name for serial device locking protocol. Is it from uucp? But its weirdness makes it memorable, unlike a generic English word like `property'. Better yet, I don't have to quote it here. - f. Stdio's prefix meaning `file'. To fit indentifiers in 8 characters, it can't even have an underscore. - pw_. Old prefix/abbrieviation for `password'. It's more readable than `password' once you are used to it. - gr_. Newer prefix for `group'. More verbose than the g in gid. - quota_. At least the English word is short. Just noticed some more disorder: the groups of the defines at the end are in random (mostly historical) order (U*, HO*, F*, PW*, HN* (for the last parameter of humanize_number()), HN* (for the second last parameter...), HD*. If the pidfile API had defines and if the API is kept in its own section, its defines should be in that section. Most of the other APIs that have a man page are large enough to deserve the same treatment if it is done for pidfile. Some like dehumanize^Wscientificize^W humanize_number() are larger although they have fewer functions, since they have lots of defines. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120115073823.O843>