libc++: move checks for newlib to actually work

[LittleFox94]

Summary

The checks did not work in __config, since no header defining _NEWLIB_VERSION was included before. This patch moves the two checks for newlib to the headers that actually need it - and after they already include relevant headers.

[LittleFox94]

Since this include was deleted as "dead code" before, I added a comment why it is not dead

[EricWF]

libcxx/include/__config

13

Please do this later in the file,

Specifically after we #define __has_include in the case where we don't have it.

[ldionne]

I am slightly concerned that this also introduces a bunch of declarations in __config, which is the most included file of the library. That would also be the first time we include unistd.h in the libc++ headers (as opposed to the sources used to build the dylib). That might not be an issue, but it's something to keep in mind.

Is this necessary for libc++ to work with Newlib? I thought we already supported it as-is (we certainly have some code paths that are conditionals to it).

[LittleFox94]

In D79888#2034706, @ldionne wrote:

I am slightly concerned that this also introduces a bunch of declarations in __config, which is the most included file of the library. That would also be the first time we include unistd.h in the libc++ headers (as opposed to the sources used to build the dylib). That might not be an issue, but it's something to keep in mind.

Another way would be to include _newlib_version.h if it exists, would that be better? Would then be a newlib specific patch.

Is this necessary for libc++ to work with Newlib? I thought we already supported it as-is (we certainly have some code paths that are conditionals to it).

__config has a check for newlib which never works, due to the preprocessor macro not being set without an include (line 933 in the patched revision)

[EricWF]

We could lift the two macros that depend on _NEWLIB_VERSION into the headers that use them.
Then we'll avoid the include in config.

[LittleFox94]

In D79888#2034859, @EricWF wrote:

We could lift the two macros that depend on _NEWLIB_VERSION into the headers that use them.
Then we'll avoid the include in config.

sounds good to me. If this is also good for @ldionne I'll prepare a new patch :)

[ldionne]

In D79888#2034894, @LittleFox94 wrote:

In D79888#2034859, @EricWF wrote:

We could lift the two macros that depend on _NEWLIB_VERSION into the headers that use them.
Then we'll avoid the include in config.

sounds good to me. If this is also good for @ldionne I'll prepare a new patch :)

Yeah, I'd like to see that patch.

[ldionne]

Is this review still relevant? If not, let's abandon it to clear up the review queue.

[LittleFox94]

In D79888#2272669, @ldionne wrote:

Is this review still relevant? If not, let's abandon it to clear up the review queue.

Ouch oops.. sorry, was really scatterbrained this year .. I'll look into it in the next 2 days, feel free to close afterwards if you don't read from me

Again, sorry

[ldionne]

LGTM with nitpick. Once updated, do you need someone to commit this for you? If so, please provide Author Name <email@domain> for attribution. Thanks!

libcxx/include/fstream

201

You can remove this level of #if, we never expect users to define it, and I'd rather not give the impression that we allow users to.

[LittleFox94]

Removed an unnecessary check as requested in review

[LittleFox94]

Removed the unnecessary check as requested.

I cannot commit myself, please use "Mara Sophie Grosch <[e-mail]>" as commit author. Thank you and sorry it took so long.

libcxx/include/__config

 // -*- C++ -*-
 //===--------------------------- __config ---------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP_CONFIG
 #define _LIBCPP_CONFIG
 
 #include <__config_site>
 
 #if defined(_MSC_VER) && !defined(__clang__)
 #  if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #    define _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER
 #  endif
 #endif
 
 #ifndef _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER

 #define _LIBCPP_EXTERN_TEMPLATE_DEFINE(...) template __VA_ARGS__;
 #endif
 
 #if defined(__APPLE__) || defined(__FreeBSD__) || defined(_LIBCPP_MSVCRT_LIKE) || \
     defined(__sun__) || defined(__NetBSD__) || defined(__CloudABI__)
 #define _LIBCPP_LOCALE__L_EXTENSIONS 1
 #endif
 
-#if defined(__unix__) || (defined(__APPLE__) && defined(__MACH__))
-// Most unix variants have catopen.  These are the specific ones that don't.
-#  if !defined(__BIONIC__) && !defined(_NEWLIB_VERSION)
-#    define _LIBCPP_HAS_CATOPEN 1
-#  endif
-#endif
-
 #ifdef __FreeBSD__
 #define _DECLARE_C99_LDBL_MATH 1
 #endif
 
 // If we are getting operator new from the MSVC CRT, then allocation overloads
 // for align_val_t were added in 19.12, aka VS 2017 version 15.3.
 #if defined(_LIBCPP_MSVCRT) && defined(_MSC_VER) && _MSC_VER < 1912
 #  define _LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION

 #if !__has_builtin(__builtin_addressof) && _GNUC_VER < 700
 #define _LIBCPP_HAS_NO_BUILTIN_ADDRESSOF
 #endif
 
 #if !__has_builtin(__builtin_is_constant_evaluated) && _GNUC_VER < 900
 #define _LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED
 #endif
 
-#if !defined(_LIBCPP_HAS_NO_OFF_T_FUNCTIONS)
-#  if defined(_LIBCPP_MSVCRT) || defined(_NEWLIB_VERSION)
-#    define _LIBCPP_HAS_NO_OFF_T_FUNCTIONS
-#  endif
-#endif
-
 #if __has_attribute(diagnose_if) && !defined(_LIBCPP_DISABLE_ADDITIONAL_DIAGNOSTICS)
 #  define _LIBCPP_DIAGNOSE_WARNING(...) \
      __attribute__((diagnose_if(__VA_ARGS__, "warning")))
 #  define _LIBCPP_DIAGNOSE_ERROR(...) \
      __attribute__((diagnose_if(__VA_ARGS__, "error")))
 #else
 #  define _LIBCPP_DIAGNOSE_WARNING(...)
 #  define _LIBCPP_DIAGNOSE_ERROR(...)

libcxx/include/fstream

 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_PUSH_MACROS
 #include <__undef_macros>
 
+#if defined(_LIBCPP_MSVCRT) || defined(_NEWLIB_VERSION)
+#  define _LIBCPP_HAS_NO_OFF_T_FUNCTIONS
+#endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 template <class _CharT, class _Traits>
 class _LIBCPP_TEMPLATE_VIS basic_filebuf
     : public basic_streambuf<_CharT, _Traits>
 {
 public:

libcxx/include/locale

 #include <limits>
 #include <version>
 #ifndef __APPLE__
 #include <cstdarg>
 #endif
 #include <cstdlib>
 #include <ctime>
 #include <cstdio>
-#ifdef _LIBCPP_HAS_CATOPEN
-#include <nl_types.h>
-#endif
+
+#if defined(__unix__) || (defined(__APPLE__) && defined(__MACH__))
+// Most unix variants have catopen.  These are the specific ones that don't.
+#  if !defined(__BIONIC__) && !defined(_NEWLIB_VERSION)
+#    define _LIBCPP_HAS_CATOPEN 1
+#    include <nl_types.h>
+#  endif
+#endif
 
 #ifdef _LIBCPP_LOCALE__L_EXTENSIONS
 #include <__bsd_locale_defaults.h>
 #else
 #include <__bsd_locale_fallbacks.h>
 #endif
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)