svn commit: r230037 - head/lib/libutil

Bruce Evans brde at optusnet.com.au
Tue Jan 17 08:44:48 UTC 2012


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 <tab> 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
<float.h>, 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


More information about the svn-src-all mailing list