Re: [PATCH 2/4] Minimize sysdeps code involved in defining major/minor/makedev.

[Zack Weinberg <zackw at panix dot com>]

On Wed, Jun 22, 2016 at 11:47 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On 13 May 2016 09:26, Zack Weinberg wrote:
>> --- /dev/null
>> +++ b/bits/sysmacros.h
>> +
>> +/* dev_t in glibc is a 64-bit quantity, with 32-bit major and minor numbers.
>> +   Our default encoding is MMMM Mmmm mmmM MMmm, where M is a hex digit of
>> +   the major number and m is a hex digit of the minor number.  This is
>> +   downward compatible with legacy systems where dev_t is 16 bits wide,
>> +   encoded as MMmm.  It is also downward compatible with the Linux kernel,
>> +   which (as of 2016) uses 32-bit dev_t, encoded as mmmM MMmm.
>> +
>> +   Systems that use an incompatible encoding for dev_t should override this
>> +   file in the appropriate sysdeps subdirectory.  The macros __major_body,
>> +   __minor_body, and __makedev_body are used as the bodies of inline
>> +   functions, and their arguments are guaranteed to be the names of
>> +   parameter variables, so it is safe to use them multiple times and
>> +   unnecessary to parenthesize them.  See sys/sysmacros.h for details.  */
>> +
>> +#define __major_body(__dev_)                                    \
>
> the args don't need the __ prefixes since they're expanded by the
> preprocessor.  so here you can just use "dev".

See below.

>> +  unsigned int __major_;                                        \
>
> i think stylewise, we don't put trailing underscores.  was there
> something specific you were trying to avoid ?

This is poor man's macro hygiene.  Symbols that are declared by the
expansion of a macro might shadow or collide with symbols declared in
the context where the macro is expanded.  Precisely since trailing
underscores are not used anywhere else, adding a trailing underscore
to these symbols minimizes the risk of such a collision (but does not
100% prevent it).

The variable must also have a leading double underscore to put it into
the implementation namespace, lest it collide with an
application-defined  _macro_ named "major" (with or without a trailing
underscore).

As you say, none of this is necessary for the formal parameter name
"[__]dev[_]", but I feel that it is appropriate to do it anyway so
that local variables and arguments are on equal footing.  So I would
rather leave that alone.

In the alternative we could go back to the earlier version of the
patch, in which these were expressions not function bodies.

>> +  __major_  = ((__dev_ & (__dev_t) 0x00000000000fff00u) >>  8); \
>> +  __major_ |= ((__dev_ & (__dev_t) 0xfffff00000000000u) >> 32); \
>
> parens around (__dev_)

Note the comment above

>> +  [...] The macros __major_body,
>> +   __minor_body, and __makedev_body are used as the bodies of inline
>> +   functions, and their arguments are guaranteed to be the names of
>> +   parameter variables, so it is safe to use them multiple times and
>> +   unnecessary to parenthesize them.  See sys/sysmacros.h for details.

Given this, do you insist on defensive parentheses?  I think this code
is already plenty hard to read without it.

zw