svn commit: r298247 - head/sbin/fdisk_pc98

[Bruce Evans]

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.

>> 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.

> I came up with something like:
>
>        struct  part_type *ptr = part_types;
>
>        for (int i = 0; i < nitems(part_types); i++) {
>                if (ptr->type == (type & 0x7f))
>                        return (ptr->name);
>                ptr++;
>        }
>        return("unknown");
>
> What do you think?

The first line preserves 3 style bugs that were fixed in jhb's version:
- tab after 'struct'
- tab (indentation) anywhere at all in the local declarations
- initialization in declararions.

All the declarations in this function had the second 2 of these style
bugs.  Removing them thus fixes 4 style bugs.

The next non-empty line adds 3 style bugs:
- nested declaration
- C++ syntax needed for nested declaration/initialization
- inconsistent scoping style.  'ptr' is just as local to the loop as
   'i'

The loop body preserves verbosesness and inconsistent style that were
fixed in jhb's version.  Just like for the initialization, incrementing
the pointer is just as fundamental to the loop's operation as incrementing
the counter (slightly more so).

Normal style is probably still to increment pointers independently of
indexes and write this as 'i++, p++' in the loop statement, but this
uses pointers excessively.  It was good on PDP11's with primitive
compilers.  It is clearer to only increment i and use indexing for
the pointer: base_p[i], and depend on the compiler to change this
to p++ if this is good, but on modern CISC CPUs like i386 the p++
method is a pessimization unless the compiler does the opposite
transformation.  Here base_p is a global variable so we don't even
need another variable (less the compiler copy it to a register iff
that is good).

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");

The number of references to part_types cannot be reduced much by using a
local variable, because part_types is an array but the local can only
be a pointer, and nitems() needs the array.  All the other versions have
another style bug related to this: they write the initialization as
'ptr = part_types'.  This depends on the array decaying to a pointer.
This expression would still work if part_types were already a pointer,
but nitems(part_types) wouldn't work then.  The initialization should
be written as 'ptr = &part_types[0]' to make the types clearer.  But it
is better to never convert arrays to pointers.

Bruce