Re: [PATCH v3 01/24] Feature test macros overhaul: sys/features.h

[Craig Howland <howland at LGSInnovations dot com>]

On 03/14/2016 11:10 PM, Yaakov Selkowitz wrote:
> This is the complete rework of the feature tests macros for better
> compatibility with GNU libc, primarily based on the Linux man pages
> documentation:
>
> http://man7.org/linux/man-pages/man7/feature_test_macros.7.html
>
> The previous implementation was flawed in its approach that macros were
> often used to hide symbols if defined (e.g. !defined __STRICT_ANSI__ or
> !defined _POSIX_SOURCE), whereas the approach of glibc is that these macros
> make symbols available when defined (e.g. defined _BSD_SOURCE, or as used
> internally, #if __BSD_VISIBLE).  As much open-source software is written
> with glibc in mind, this necessitated patching numerous packages just to
> compile.
>
> In particular, __STRICT_ANSI__ (which is defined by gcc -ansi or -std=c*)
> was given too much importance.  This implementation limits the influence
> of __STRICT_ANSI__ to controlling the default when no other feature test
> macros are defined, and to the inclusion of <alloca.h> in <stdlib.h> as
> documented.  These are the only places where __STRICT_ANSI__ should be
> tested.
>
> The following macros are now accepted: _ATFILE_SOURCE, _BSD_SOURCE,
> _DEFAULT_SOURCE, _ISOC99_SOURCE, _ISOC11_SOURCE, _LARGEFILE_SOURCE,
> _SVID_SOURCE, _XOPEN_SOURCE_EXTENDED.
>
> The existing __*_VISIBLE internal macros have been kept mostly
> compatible with the original BSD implementation, with some changes to
> the criteria which controls them.  Several more macros in this style
> have been added where needed for concision or accuracy.
>
> Enabling C++11 or newer in the compiler also enables C99 and C11
> functions.  Doing so should help move away from the need to define
> _GNU_SOURCE in g++ for _GLIBCXX_USE_C99 support as on Linux:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51749
>
> Signed-off-by: Yaakov Selkowitz <yselkowi@redhat.com>
> ---
>   newlib/libc/include/sys/cdefs.h    | 126 ------------------------
>   ...
     This all makes things more consistent (which is sorely needed), but it 
seems to be lacking one major aspect:  documentation in the source.  The email 
points to FEATURE_TEST_MACROS(7), but the newlib developers and users are in the 
dark as to how to properly use this new method--the man page is not mentioned 
anywhere in a file, for example.  The prior method at least partly used 
user-level defines that were self-explanatory, and this actually takes a step 
backwards from that.  For example, __STRICT_ANSI is a compiler predefine that 
can be looked up, and _POSIX_C_SOURCE can be found in POSIX. However, the user 
is now confronted with the *_VISIBLE defines, but just what these mean is not 
clear.  (Some are more so than others, as ISOC99_SOURCE is pretty obvious, but 
what does ATFILE_VISIBLE mean?)  In a similar manner, if a developer adds a 
function, what is the proper way to label it with *VISIBLE?  Yaakov clearly had 
a method by which this whole thing was done, and knowing how for others 
following on is needed to avoid divergence.  In addition, not knowing the plan 
for guarding makes it rather difficult to review the patches for correctness.
     The comments at the start of features.h give a reasonable summary for an 
informed user (so there is something on the user level), but developer guidance 
on using VISIBLE is essentially non-existent.  (Sure, you can try and wade 
through all the define chains, but that is prone to error and some interpretation.)
     So in short, this would benefit greatly from a 2-part usage section being 
added somewhere in the code.  I'd think that it should point to 
FEATURE_TEST_MACROS(7) as the email did, and have guidance for users (to get 
this feature, do this), and for developers (use this algorithm to choose your 
guards).  Perhaps this all would belong in features.h, or a brief version with a 
pointer to some kind of README file.  And all of it really should end up in the 
documentation.  I'd think that as a bare minimum, a reference to 
FEATURE_TEST_MACROS(7) ought to be added, and more added later. (But that 
approach risks nothing being added, and things thereby being a little more 
likely to diverge.)
                Craig