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

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius marked 2 inline comments as done.
curdeius 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__)
----------------
Quuxplusone wrote:
> @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.
There's one place only where I used `_LIBCPP_APPLE_CLANG_VER` instead of `__apple_build_version__`, line 513.
It should be pretty straightforward.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99515