PR32476: __nop_locale_mgmt.h not needed with newlib 2.5+

[waltl]

Summary

newlib 2.5 added the locale management functions, and so our stub __nop_locale_mgmt.h shouldn't be included, as it conflicts with newlibs definitions. newlib 2.4 and earlier still need the header though.

Patch by Martin J. O'Riordan

[orivej]

include/support/newlib/xlocale.h

20 ↗

(On Diff #95517)

You meant __NEWLIB_MINOR__ < 5.
Could not this be just the following?

#if __NEWLIB__ < 2 || __NEWLIB__ == 2 && __NEWLIB_MINOR__ < 5
#include <support/xlocale/__nop_locale_mgmt.h>
#endif

[bcraig]

include/support/newlib/xlocale.h

20 ↗

(On Diff #95517)

I suspect you are right. Here's the trouble though... I don't have a good way to test this. I spent a couple of days trying to get newlib building on my machine, and I ended up giving up. So I just took Martin J. O'Riordan's patch from here:
http://clang-developers.42468.n3.nabble.com/Re-Newlib-v2-5-0-and-LibC-locale-support-BROKEN-tt4054882.html

Are you in a good position to test this change? Bonus points if you can try newlib 2.4 and 2.5.

[MartinO]

The introduction of the macros 'NEWLIB' and 'NEWLIB_MINOR' appear to have been first introduced in Newlib v2.3.0.20160226, but the new interface for '<locale.h>' was introduced in Newlib v2.4.0.20160923 - previous releases of v2.4.0 did not make these changes.

This is why the tests 'defined(NEWLIB)' and 'defined(NEWLIB_MINOR)' are also necessary.

Unfortunately, I don't know of anyway of telling if Newlib v2.4.0 is the 20160923 version or an earlier version such as the original v2.4.0 release or the 20160527 version which was the last version of Newlib that used the old interface. There is no macro that provides this information and 'NEWLIB_PATCHLEVEL' is always '0' for each is these intermediate versions (I don't know why they don't bump the patch number).

I've no comment about whether or not the '__POSIX_VISIBLE' test is required, it was already present and I simply supplemented it with the Newlib version checks.

MartinO

[waltl]

Thanks for submitting the patch -- I will make the necessary revision and test it.

[waltl]

PR32476: __nop_locale_mgmt.h not needed with newlib 2.5+

Newlib 2.5 added the locale management functions, so it should not
include __nop_local_mgmt.h. This change adds proper guard around that
include statement.

For newlib 2.4, some releases contain these functions and some don't,
and they all have the same version numbers. This patch will work
properly with the initial "2.4.0" release which does not include these
functions and require __nop_local_mgmt.h.

I've tested this against newlib 2.2 and 2.5, and also sanity check
against other different version numbers.

[mclow.lists]

This change looks fine to me - but I don't have the different newlib versions to test against.
Once everyone else is happy with the version detection, then I'm good with this.

[MartinO]

Looks good. Previously I had also got a check for 'defined(__NEWLIB_MINOR__)' but I realise that was not necessary

[bcraig]

LGTM.
@waltl : Do you need me to submit the changes, or will you handle that?

[waltl]

In D32146#780231, @bcraig wrote:

LGTM.
@waltl : Do you need me to submit the changes, or will you handle that?

Thanks -- I think jyknight will handle it.

libcxx/trunk/include/support/newlib/xlocale.h

 #define _LIBCPP_SUPPORT_NEWLIB_XLOCALE_H
 
 #if defined(_NEWLIB_VERSION)
 
 #include <cstdlib>
 #include <clocale>
 #include <cwctype>
 #include <ctype.h>
+#if !defined(__NEWLIB__) || __NEWLIB__ < 2 || \
+    __NEWLIB__ == 2 && __NEWLIB_MINOR__ < 5
 #include <support/xlocale/__nop_locale_mgmt.h>
+#endif
 #include <support/xlocale/__posix_l_fallback.h>
 #include <support/xlocale/__strtonum_fallback.h>
 
 #endif // _NEWLIB_VERSION
 
 #endif