From owner-svn-src-all@FreeBSD.ORG Sat Jan 14 21:02:34 2012 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E5A841065670; Sat, 14 Jan 2012 21:02:34 +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 7899C8FC0C; Sat, 14 Jan 2012 21:02:34 +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 q0EL2Uin020035 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 15 Jan 2012 08:02:32 +1100 Date: Sun, 15 Jan 2012 08:02:30 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Pawel Jakub Dawidek In-Reply-To: <20120114182758.GJ1694@garage.freebsd.pl> Message-ID: <20120115073823.O843@besplex.bde.org> References: <201201122249.q0CMnaZe030200@svn.freebsd.org> <20120114204720.Q1458@besplex.bde.org> <20120114182758.GJ1694@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, Guy Helmer , Bruce Evans Subject: Re: svn commit: r230037 - head/lib/libutil X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 14 Jan 2012 21:02:35 -0000 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 . 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