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

[Zack Weinberg <zackw at panix dot com>]

On 05/05/2016 12:15 AM, Mike Frysinger wrote:
> On 28 Apr 2016 21:39, Zack Weinberg wrote:
>> New ports henceforth need only provide bits/sysmacros.h defining
>> macros __dev_makedev, __dev_major, and __dev_minor, and it's OK if
>> these macros evaluate their arguments more than once.
> 
> as a general policy, i don't think we want that.

Hmm.  The only obvious alternative is to make the macros be function
bodies.  On the one hand, that's uglier; on the other hand, that would
discourage people from using them directly (__names only go so far).
I'll try that in the next revision and you can see what you think.

>> 1) There seem to be generic "bits" headers in a top-level directory
>> named bits/, as well as sysdeps/generic/bits/ which is where I would
>> expect them to be.  What's up with that?
> 
> i suspect it's something like:
> - bits/: The One True header that is used everywhere and overriding
>   doesn't make sense.
> - sysdeps/generic/bits/: We expect platforms to override it, so throw
>   it in here to signal as much.

addressed separately

>> Does it maybe make sense to promote
>> the extended dev_t encoding glibc uses on all Linux-based
>> configurations, which supports 32-bit major and minor numbers, to
>> generic?
>
> i think making it 64-bit as a GNU extesion/base makes sense

OK, I'll make that change.  I'll have to leave Hurd potentially broken,
though.

>> +/* Confirm that makedev (major (d), minor (d)) == d. */
> 
> GNU style says two spaces after periods.  comes up a few times.

argh

>> +      printf ("FAIL: %04lx != %04lx (maj %02x min %02x)\n",
>> +              d2, d1, maj, min);
> 
> you noted that we can't rely on dev_t size, so you'll probably want to
> cast to uint64_t and use PRIx64 in the format string.

Good point.  I think it's always 64 bits _wide_, but it isn't
necessarily always matched to %lx.

> also, you seem to be omitting the 8 spaces -> 1 tab conversion here and
> in other places in this patch.

Sorry, I mostly work on projects that want _no_ tabs these days so I
have my editor set to always use spaces.

>> +#define __dev_makedev(major, minor) (((major) << 8) | (minor & 0xff))
> 
> parens around (minor) please

This file will not exist in the revision, but I've double-checked for
other such errors.

zw

Attachment: signature.asc
Description: OpenPGP digital signature