[libcxx-commits] [PATCH] D151900: [libc++][NFC] Create a new folder for config-related headers.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__config_dir/base.h:21-49
+// Aligned allocation.
+//
+// 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
+#  elif defined(_LIBCPP_ABI_VCRUNTIME) && !defined(__cpp_aligned_new)
----------------
This could move to `aligned_allocation.h` or `features.h`.


================
Comment at: libcxx/include/__config_dir/base.h:51-58
+// Apple.
+#  if defined(_LIBCPP_COMPILER_CLANG_BASED)
+
+#    if defined(__APPLE__) && !defined(__i386__) && !defined(__x86_64__) && (!defined(__arm__) || __ARM_ARCH_7K__ >= 2)
+#      define _LIBCPP_ABI_ALTERNATE_STRING_LAYOUT
+#    endif
+
----------------
Let's move this to `abi.h`.


================
Comment at: libcxx/include/__config_dir/base.h:100-110
+#  if defined(__APPLE__) || defined(__FreeBSD__) || defined(_LIBCPP_MSVCRT_LIKE) || defined(__NetBSD__)
+#    define _LIBCPP_LOCALE__L_EXTENSIONS 1
+#  endif
+
+#  if defined(__APPLE__) || defined(__FreeBSD__)
+#    define _LIBCPP_HAS_DEFAULTRUNELOCALE
+#  endif
----------------
`__config_dir/locale.h`?


================
Comment at: libcxx/include/__config_dir/base.h:112-116
+// TODO(varconst): currently, there are bugs in Clang's intrinsics when handling Objective-C++ `id`, so don't use
+// compiler intrinsics in the Objective-C++ mode.
+#  ifdef __OBJC__
+#    define _LIBCPP_WORKAROUND_OBJCXX_COMPILER_INTRINSICS
+#  endif
----------------
We can move this to `objc.h`, I think.


================
Comment at: libcxx/include/__config_dir/base_umbrella.h:13
+
+// This header transitively includes the most commonly used, dependency-free headers.
+
----------------
Mordante wrote:
> Why do we need this header?
> If we need it we need to think of a better name, umbrella is not descriptive.
I think the intent is to avoid the issue where you're using a macro but forget to include the header that defines it. I think we should instead find a way to catch that systematically and not introduce this header, just IWYU from all the config headers.


================
Comment at: libcxx/include/__config_dir/base_umbrella.h:17
+#include <__config_dir/compiler_shims.h>
+#include <__config_dir/features.h>
+#include <__config_dir/object_format.h>
----------------
var-const wrote:
> Perhaps `features.h`, `object_format.h` and `versions.h` should be merged into e.g. `environment.h`?
IMO this is fine as-is, at least to begin with.


================
Comment at: libcxx/include/__config_dir/c_std_library.h:40-43
+#  if defined(__BIONIC__) || defined(__NuttX__) || defined(__Fuchsia__) || defined(__wasi__) ||                        \
+      defined(_LIBCPP_HAS_MUSL_LIBC) || defined(__OpenBSD__)
+#    define _LIBCPP_PROVIDES_DEFAULT_RUNE_TABLE
+#  endif
----------------
I think this would belong to `__config_dir/locale.h`.


================
Comment at: libcxx/include/__config_dir/versions.h:30
+
+#ifdef __cplusplus
+
----------------
Mordante wrote:
> Why not at the top?
I think we need the compiler detection to be enabled even when we get included in C mode. That was for `<stdatomic.h>` IIRC.

Honestly my preference would be for all of `__config_dir` to be include-able in C mode, but I'd do that in a follow-up.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151900/new/

https://reviews.llvm.org/D151900