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

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Tue Mar 30 09:27:04 PDT 2021


ldionne 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.
If we disregard the need to make `-Wundef` work, we get:

```
#if __has_keyword(__is_fundamental) &&                                         \
    !(_LIBCPP_CLANG_VER < 1000) &&               \
    !(_LIBCPP_APPLE_CLANG_VER < 1205) &&                                       \
    !defined(_LIBCPP_CXX03_LANG)
```

I think that's really straightforward, so I think that design is good. I agree it doesn't play nicely with `-Wundef` though, as it adds a bunch of syntactic noise. In fact, I do think that `-Wundef` is just generally noisy.

One option (which you suggested before) would be to define macros like `_LIBCPP_CLANG_VER_LESSTHAN(xxx)` that is considered false if `_LIBCPP_CLANG_VER` isn't defined. We'd have to do it for each compiler we support, though (so we'd also have `_LIBCPP_APPLE_CLANG_VER_LESSTHAN` and `_LIBCPP_GCC_VER_LESSTHAN`, etc.).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99515



More information about the libcxx-commits mailing list