Re: RFC: deprecate sys/sysmacros.h inclusion from sys/types.h

[Zack Weinberg <zackw at panix dot com>]

Thanks for the review. I will make revisions this weekend.

On Thu, Apr 21, 2016 at 3:31 PM, Roland McGrath <roland@hack.frob.com> wrote:
> This patch has no ChangeLog entries.

I'll add those after all revisions are complete.

> Please drop the clang-related changes to sys/cdefs.h in this patch.
> Adding __attribute_deprecated_msg__ here is OK, but other changes to
> sys/cdefs.h belong in a separate change.

OK, I'll split it.

>> --- /dev/null
>> +++ b/include/sys/sysmacros.h
>> @@ -0,0 +1,3 @@
>> +#ifndef _SYS_SYSMACROS_H
>> +#include <misc/sys/sysmacros.h>
>> +#endif
>
> No #ifndef here.  include/ files should be nothing but direct
> wrappers when that works, as it should here.

OK.

>> --- /dev/null
>> +++ b/misc/makedev.c
>
> Moving this file to OS-independent place means you also need to add
> the functions to misc/Versions.  Since they didn't exist before on
> non-Linux configurations, misc/Versions should list them under
> GLIBC_2.24.  The existing sysdeps/unix/sysv/linux/Versions entries
> will give them the old symbol version in Linux configurations.

OK.

>> --- /dev/null
>> +++ b/misc/sys/sysmacros.h
>
> Throughout this file you should indent nested preprocessor
> directives, e.g.:
>
> #ifdef foo
> # define bar ...
> #endif

OK.

>> +#define __INCLUSION_DEPRECATION_MSG(symbol)                                  \
>
> Use a name that starts with "__SYSMACROS_".

OK.

>> +  "\n  For compatibility with BSD, it is currently defined by <sys/types.h>" \
>> +  "\n  as well, but we plan to remove this soon.  To use `" #symbol "',"     \
>> +  "\n  include <sys/sysmacros.h> directly.  If you did not intend to use"    \
>> +  "\n  a system-defined macro `" #symbol "', you can suppress it by "        \
>> +  "\n  defining the macro _SYS_TYPES_NO_SYSMACROS (with any value) before "  \
>> +  "\n  including any system headers."
>
> Say "historical compatibility" rather than "compatibility with BSD".

OK.

> I'm not convinced that we should support the _SYS_TYPES_NO_SYSMACROS
> macro at all.  If we advise applications to define that, then it will
> litter random sources for years to come.  But it serves only a brief
> transitional purpose.  And IMHO there is really no need for that
> option at all.  Things will continue to work as they did today with
> no warnings for applications that were not using these symbols.

Hmm, good point.  The macro was for programs that *are* being broken
because they didn't expect sys/types.h to define major/minor/makedev,
and advising such programs to #undef those symbols is better advice,
since that will work on *BSDs that still define them there.

>> +#ifdef __USE_EXTERN_INLINES
>> +#define __SYSMACRO_IMPL(rtype, name, proto, expr) \
>
> An example of unintended preprocessor directives.

I assume you meant unindented.

>> +  __SYSMACRO_DECL(rtype, name, proto) \
>
> Space before paren here.

Doh.  (I've been working mostly on programs where the preferred style
is different, lately.)

>> +__SYSMACRO_IMPL(unsigned int, major, (__dev_t __dev), __dev_major (__dev))
>> +__SYSMACRO_IMPL(unsigned int, minor, (__dev_t __dev), __dev_minor (__dev))
>> +__SYSMACRO_IMPL(__dev_t, makedev,
>> +                (unsigned int __major, unsigned int __minor),
>> +                __dev_makedev (__major, __minor))
>
> Space before parens here.
>
> Also, make all these macros __SYSMACROS_* instead of _SYSMACRO_* (so
> the prefix matches the name of the header file).

I think singular is more appropriate because it expands to the
implementation of *one* sysmacro.

>> +#ifdef _DEPRECATED_INCLUSION_OF_SYS_SYSMACROS_H
>> +#define major(dev) gnu_dev_major_from_sys_types (dev)
>> +#define minor(dev) gnu_dev_minor_from_sys_types (dev)
>> +#define makedev(maj, min) gnu_dev_makedev_from_sys_types (maj, min)
>
> I think these compat wrapper functions should have __ names.

OK.

zw