This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]