This is the mail archive of the
mailing list for the glibc project.
Re: RFC: deprecate sys/sysmacros.h inclusion from sys/types.h
- From: Zack Weinberg <zackw at panix dot com>
- To: Roland McGrath <roland at hack dot frob dot com>
- Cc: GNU C Library <libc-alpha at sourceware dot org>
- Date: Thu, 21 Apr 2016 16:59:53 -0400
- Subject: Re: RFC: deprecate sys/sysmacros.h inclusion from sys/types.h
- Authentication-results: sourceware.org; auth=none
- References: <564D4C62 dot 3090404 at panix dot com> <20160421193125 dot EC9C42C3B00 at topped-with-meat dot com>
Thanks for the review. I will make revisions this weekend.
On Thu, Apr 21, 2016 at 3:31 PM, Roland McGrath <email@example.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>
> No #ifndef here. include/ files should be nothing but direct
> wrappers when that works, as it should here.
>> --- /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.
>> --- /dev/null
>> +++ b/misc/sys/sysmacros.h
> Throughout this file you should indent nested preprocessor
> directives, e.g.:
> #ifdef foo
> # define bar ...
>> +#define __INCLUSION_DEPRECATION_MSG(symbol) \
> Use a name that starts with "__SYSMACROS_".
>> + "\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".
> 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.