[libcxx-commits] [PATCH] D99515: [libc++] Build and test with -Wundef warning. NFC.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__config:189
+#  define _LIBCPP_COMPILER_CLANG_BASED
+#  define _LIBCPP_CLANG_VER (__clang_major__ * 100 + __clang_minor__)
 #elif defined(__GNUC__)
----------------
@ldionne wrote:
```
_LIBCPP_COMPILER_CLANG_BASED // defined whenever the compiler is based on Clang, i.e. LLVM Clang or Apple Clang (but there could be others). This is used to check for general capabilities available on Clang-based compilers like a few extensions or attributes.
_LIBCPP_CLANG_VER // defined to the version of LLVM Clang if the compiler is LLVM Clang, undefined otherwise
_LIBCPP_APPLE_CLANG_VER // defined to the version of Apple Clang if the compiler is Apple Clang, undefined otherwise
```
I think this fits in well with the existing style, but I'm a little worried about how it plays with //this patch specifically//. Combined with `-Wundef`, this will lead to pretty complicated conditions. Like, let's say we wanted to enable `__is_fundamental` on Clang 10+ and Apple Clang 12.5+:
```
#if __has_keyword(__is_fundamental) &&                                         \
    !(defined(_LIBCPP_CLANG_VER) && _LIBCPP_CLANG_VER < 1000) &&               \
    !(defined(_LIBCPP_APPLE_CLANG_VER) && _LIBCPP_APPLE_CLANG_VER < 1205) &&   \
    !defined(_LIBCPP_CXX03_LANG)
```
Or let's say we wanted to enable `__is_fundamental` on Clang 10+ and never on Apple Clang (which is the current state of the art, because we don't know the Apple Clang version that made this work):
```
#if __has_keyword(__is_fundamental) &&                                         \
    !(defined(_LIBCPP_CLANG_VER) && _LIBCPP_CLANG_VER < 1000) &&               \
    !defined(_LIBCPP_APPLE_CLANG_VER) &&                                       \
    !defined(_LIBCPP_CXX03_LANG)
```
This isn't too bad, I guess; but it's complicated enough that we should audit the final version of this PR //very// carefully to make sure none of the existing conditions broke or changed behavior accidentally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99515