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: Mike Frysinger <vapier at gentoo dot org>, GNU C Library <libc-alpha at sourceware dot org>
- Date: Fri, 24 Jun 2016 11:04:42 -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> <CAKCAbMg7+XJueuexMuUDfKoEX_B3uhk6reT5Ls2k=SMtDR-K4w at mail dot gmail dot com>
On Wed, Jun 22, 2016 at 1:00 PM, Zack Weinberg <zackw@panix.com> wrote:
> 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.
I need you to answer these questions before I can rework the patch.
zw