From owner-svn-src-head@FreeBSD.ORG Tue Jan 17 08:44:48 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 E3780106566B; Tue, 17 Jan 2012 08:44:47 +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 75CEA8FC12; Tue, 17 Jan 2012 08:44:47 +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 q0H8ifKO002654 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 17 Jan 2012 19:44:44 +1100 Date: Tue, 17 Jan 2012 19:44:41 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Guy Helmer In-Reply-To: <52A73054-9960-403B-B2FE-857C8801D129@palisadesystems.com> Message-ID: <20120117190611.F1710@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> 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, Pawel Jakub Dawidek , Bruce Evans 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 08:44:48 -0000 On Mon, 16 Jan 2012, Guy Helmer wrote: > On Jan 14, 2012, at 3:02 PM, Bruce Evans 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. I see you committed it. It is mostly OK, but as usual a pre-commit review would have been useful. > Index: lib/libutil/libutil.h > =================================================================== > --- lib/libutil/libutil.h (revision 230221) > +++ lib/libutil/libutil.h (working copy) > +/* Flags for hexdump(3) */ > #define HD_COLUMN_MASK 0xff > #define HD_DELIM_MASK 0xff00 > #define HD_OMIT_COUNT (1 << 16) > #define HD_OMIT_HEX (1 << 17) > #define HD_OMIT_CHARS (1 << 18) > > +/* Flags for humanize_number(3) flags */ The double "flags" is meaningful but hard to parse. > +#define HN_DECIMAL 0x01 > +#define HN_NOSPACE 0x02 > +#define HN_B 0x04 > +#define HN_DIVISOR_1000 0x08 > +#define HN_IEC_PREFIXES 0x10 > + > +/* Flags for humanize_number(3) scale */ It is only after reading this that the double "flags" starts making sense. Add " parameter" to the end of each to make more sense. Then omit the first "Flags " to make even more sense. Then consider adding some punctuation. Then add "Values" where "Flags" was. The first "Flags" was only a paraphrase of "`flags' parameter". This is most useful when the parameter's name is not `flags', and only works then there is only 1 parameter that takes flags. This results in: /* Values for humanize_number(3)'s `flags' parameter */ /* Values for humanize_number(3)'s `scale' parameter */ Perhaps you can abbreviate this a bit. "Values " is mostly tautologous but seems to improve readability. > +/* return values from realhostname() */ This one is not capitalized. > +/* Return values from uu_lock() */ > +#define UU_LOCK_INUSE 1 > +#define UU_LOCK_OK 0 > +#define UU_LOCK_OPEN_ERR -1 > +#define UU_LOCK_READ_ERR -2 > +#define UU_LOCK_CREAT_ERR -3 > +#define UU_LOCK_WRITE_ERR -4 > +#define UU_LOCK_LINK_ERR -5 > +#define UU_LOCK_TRY_ERR -6 > +#define UU_LOCK_OWNER_ERR -7 Er, the parentheses were not redundant for -N, since -N consists of 2 tokens. I just tried to find an example of why they are needed in , but could only find historical and contrived examples: - before C90, they were necessary to prevent - -1 becoming --1. Code like -UU_LOCK_OPEN_ERR isn't necessarily an error. For a non-flag macro FOO, -FOO just isn't an error, so FOO needed to be parenthesized it it was a negative integer. - now, 1(-1) is a syntax error, but 1-1 is not. Code like 1 UU_LOCK_OPEN_ERR should be a syntax error, but isn't if the macro expands to -1. This is unimportant compared with -FOO working, since the parentheses only give detection of an error. Parentheses are necessary for casts in macros even for C90 and later, since if FOO is defined as (long)1 then the useful expression `sizeof FOO' is the syntax error `sizeof(long) 1'. To avoid avoid worrying about this, expression-like macros should always be parenthesized iff they have more than 1 token. When worrying about this, you look at the C operator precedence table and notice that parentheses don't have the highest precedence, since they share precedence with some other operators. So there must be some magic for parenthesizing macros to be sufficient. Bruce