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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

I like the direction this is going, still somewhat hesitant since we don't have good guards against directly including subheaders. I want to think a bit about that.



================
Comment at: libcxx/include/__config_dir/attributes.h:21-33
+#  if defined(_LIBCPP_COMPILER_CLANG_BASED)
+
+#    define _LIBCPP_ALWAYS_INLINE __attribute__((__always_inline__))
+
+#    define _LIBCPP_DISABLE_EXTENSION_WARNING __extension__
+
+#  elif defined(_LIBCPP_COMPILER_GCC)
----------------
Maybe in a followup remove the blank lines.


================
Comment at: libcxx/include/__config_dir/base_umbrella.h:13
+
+// This header transitively includes the most commonly used, dependency-free headers.
+
----------------
Why do we need this header?
If we need it we need to think of a better name, umbrella is not descriptive.


================
Comment at: libcxx/include/__config_dir/c_std_library.h:11
+#ifndef _LIBCPP___CONFIG_DIR_C_STD_LIBRARY_H
+#define _LIBCPP___CONFIG_DIR_C_STD_LIBRARY_H
+
----------------
How about `libc_support.h`, when I saw c std library I thought about the c headers in the standard.


================
Comment at: libcxx/include/__config_dir/compiler_shims.h:11
+#ifndef _LIBCPP___CONFIG_DIR_COMPILER_SHIMS_H
+#define _LIBCPP___CONFIG_DIR_COMPILER_SHIMS_H
+
----------------
Likewise would I call this compiler_support.h


================
Comment at: libcxx/include/__config_dir/platform_msvc.h:11
+#ifndef _LIBCPP___CONFIG_DIR_PLATFORM_MSVC_H
+#define _LIBCPP___CONFIG_DIR_PLATFORM_MSVC_H
+
----------------
Is this MSVC or Windows in general?


================
Comment at: libcxx/include/__config_dir/versions.h:30
+
+#ifdef __cplusplus
+
----------------
Why not at the top?


================
Comment at: libcxx/include/__config_dir/versions.h:62-67
+// NOLINTEND(libcpp-cpp-version-check)
+
+// NOLINTNEXTLINE(libcpp-cpp-version-check)
+#  if __cplusplus < 201103L
+#    define _LIBCPP_CXX03_LANG
+#  endif
----------------
Maybe in a followup move C++03 in the NOLINT block.


================
Comment at: libcxx/include/__config_dir/versions.h:73-82
+#  if defined(_WIN32)
+#    define _LIBCPP_WIN32API
+// Both MinGW and native MSVC provide a "MSVC"-like environment
+#    define _LIBCPP_MSVCRT_LIKE
+// If mingw not explicitly detected, assume using MS C runtime only if
+// a MS compatibility version is specified.
+#    if defined(_MSC_VER) && !defined(__MINGW32__)
----------------
In general I sometimes feel some pats can be placed in other places, the borders are not very strong.
(I've no real suggestions for improvements, parts are just in the eye of the beholder.)

This I feel stronger about, I would like this in `platform_msvc.h`


================
Comment at: libcxx/include/module.modulemap.in:4
 // include cycle.
 module std_config [system] [extern_c] {
   textual header "__config"
----------------
We should document that `__config_dir` is excluded since it only contains macros and no named declarations.
(I assume that Clang modules also only care about named declarations.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151900