[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__)
----------------
curdeius wrote:
> ldionne wrote:
> > 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.).
> There's one place only where I used `_LIBCPP_APPLE_CLANG_VER` instead of `__apple_build_version__`, line 513.
> It should be pretty straightforward.
> If we disregard the need to make `-Wundef` work ... I think that design is good. I agree it doesn't play nicely with `-Wundef` though ...

We're on the same page. I think this design is good //except// in how it interacts with `-Wundef`. But recall that the original point of this PR is to enable `-Wundef`! So. :)

It now occurs to me that it might be even better to split "Clang" and "Apple Clang" as utterly as we split "Clang" and "GCC". So we'd have
```
_LIBCPP_COMPILER_CLANG_BASED // defined whenever the compiler is LLVM Clang or Apple Clang or maybe others
_LIBCPP_COMPILER_CLANG // defined whenever the compiler is LLVM Clang
_LIBCPP_CLANG_VER // defined to the version of LLVM Clang if the compiler is LLVM Clang, undefined otherwise
_LIBCPP_COMPILER_APPLE_CLANG // defined whenever the compiler is Apple Clang
_LIBCPP_APPLE_CLANG_VER // defined to the version of Apple Clang if the compiler is Apple Clang, undefined otherwise
```
So then my two examples would look like
```
#if __has_keyword(__is_fundamental) &&                                             \
    !(defined(_LIBCPP_COMPILER_CLANG) && _LIBCPP_CLANG_VER < 1000) &&              \
    !(defined(_LIBCPP_COMPILER_APPLE_CLANG) && _LIBCPP_APPLE_CLANG_VER < 1205) &&  \
    !defined(_LIBCPP_CXX03_LANG)
```
and
```
#if __has_keyword(__is_fundamental) &&                                         \
    !(defined(_LIBCPP_COMPILER_CLANG) && _LIBCPP_CLANG_VER < 1000) &&          \
    !defined(_LIBCPP_COMPILER_APPLE_CLANG) &&                                  \
    !defined(_LIBCPP_CXX03_LANG)
```
and I think we'd probably find little use for `_LIBCPP_COMPILER_CLANG_BASED`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99515