svn commit: r298247 - head/sbin/fdisk_pc98

[John Baldwin]

On Wednesday, April 20, 2016 01:06:38 PM Bruce Evans wrote:
> On Wed, 20 Apr 2016, Marcelo Araujo wrote:
> 
> > 2016-04-20 0:16 GMT+08:00 John Baldwin <jhb at freebsd.org>:
> >
> >> On Tuesday, April 19, 2016 04:46:13 AM Marcelo Araujo wrote:
> >>> Author: araujo
> >>> Date: Tue Apr 19 04:46:13 2016
> >>> New Revision: 298247
> >>> URL: https://svnweb.freebsd.org/changeset/base/298247
> >>>
> >>> Log:
> >>>   Remove redundant parenthesis.
> >>>
> >>>   Submitted by:       pfg
> >>>   MFC after:  2 weeks.
> 
> I don't realling like churnging to the nonstandard nitems().  Use
> of the nonstandard <sys/param.h> is bad enough.

I think it's not that bad from a readability standpoint.  Other languages
have fairly concise syntax for 'for-each' loops and this provides a
closer variant of that for statically sized arrays.  TAILQ_FOREACH() is
still nicer of course.  One could imagine doing some sort of
ARRAY_FOREACH() that was:

#define	ARRAY_FOREACH(p, array)    \
	for (size_t __i = 0, (p) = &(array)[0]; __i < nitems((array)); __i++, (p)++)

(This requires C99 to handle __i)

Perhaps better is this:

#define	ARRAY_FOREACH(p, array)	\
	for ((p) = &(array)[0]; (p) < &(array)[nitems((array))]; (p)++)

(No need for __i)

> >> For this case, it might be better to remove numentries and use
> >> nitems() directly in the one place it is used.  I would probably
> >> even do this as a for-loop:
> >>
> >>         struct part_type *ptr;
> >>         int counter;
> >>
> >>         for (counter = 0, ptr = part_types; counter < nitems(part_types);
> >>             counter++, ptr++) {
> >>                 if (ptr->type == (type & 0x7f))
> >>                         return (ptr->name);
> >>         }
> >>         return ("unknown");
> >>
> >> If you renamed 'counter' to 'i' you could probably fit it all on one line.
> 
> 'ptr' is also not a usefully verbose name.  If its name is longer than that
> of 'p', then it could more usefully give a hint of the pointer type (pp or
> ptp).
> 
> nitimems() is not even easy to use.  It is of course undocumented, but
> if we look at its internals we can see that its type is the binary
> promotion of size_t.  This type is normally size_t again, thus normally
> unsigned.  Broken compilers might warn about this.  Only broken ones
> would, since it is clear that 'counter' always has a small non-negative
> value.  Such warnings are often "fixed" by unimproving the code using
> casts.  Here the old code uses a temporary variable of type int.
> Consistently broken compilers might warn about assigning the unsigned
> expression to this signed variable.

Yes, I end up using 'unsigned' with it often due to compile warnings for
sign mismatches on comparison.

> Churnging too much (also remove excessive parentheses and braces) gives:
> 
>  	size_t i;		/* XXX: I don't like unsigned types, but... */
> 
>  	for (i = 0; i < nitems(part_types); i++)
>  		if (part_types[i].type == type & 0x7f)
>  			return (part_types[i].name);
>  	return ("unknown");

This would work for me (unless folks actually like the ARRAY_FOREACH()
idea).

-- 
John Baldwin