This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 2/4] Minimize sysdeps code involved in defining major/minor/makedev.
- From: Zack Weinberg <zackw at panix dot com>
- To: Zack Weinberg <zackw at panix dot com>, GNU C Library <libc-alpha at sourceware dot org>
- Date: Wed, 22 Jun 2016 13:00:35 -0400
- Subject: Re: [PATCH 2/4] Minimize sysdeps code involved in defining major/minor/makedev.
- Authentication-results: sourceware.org; auth=none
- References: <cover dot 1463145139 dot git dot zackw at panix dot com> <1e3999cce2297684a2828e0ef3a15ff8555dd264 dot 1463145139 dot git dot zackw at panix dot com> <20160622154759 dot GM24532 at vapier dot lan>
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