[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/type_traits:840
+    !(defined(_LIBCPP_COMPILER_CLANG) && defined(_LIBCPP_CLANG_VER) &&         \
+      _LIBCPP_CLANG_VER < 1100)
 
----------------
ldionne wrote:
> Quuxplusone wrote:
> > These are pretty icky. I would tentatively suggest at least breaking the line as
> > ```
> > #if __has_keyword(__is_pointer) &&                                             \
> >     !(defined(_LIBCPP_COMPILER_CLANG) &&                                       \
> >       defined(_LIBCPP_CLANG_VER) && _LIBCPP_CLANG_VER < 1100)
> > ```
> > but I'd also want to wait and see if anyone has a better idea (e.g. pulling out a `_LIBCPP_CLANG_VER_LESS_THAN(x)` macro or something).
> I think this is equivalent:
> 
> ```
> !(defined(_LIBCPP_CLANG_VER) && _LIBCPP_CLANG_VER < 1100)
> ```
> 
> In other words, I think checking for `_LIBCPP_COMPILER_CLANG` is redundant if you already check for `_LIBCPP_CLANG_VER` unless I'm mistaken.
@ldionne: Sadly no; see D98720. Apple Clang defines `_LIBCPP_COMPILER_CLANG` without `_LIBCPP_CLANG_VER` (because their version numbers are all screwed up), and so we need to treat Apple Clang essentially as "Clang version 0.0" for the purposes of these feature tests.

Actually, is there any appetite to just `#define _LIBCPP_CLANG_VER 0` on Apple Clang?

Double actually, @curdeius, isn't this change wrong? It should be
```
#if __has_keyword(__is_pointer) &&                                             \
    !(defined(_LIBCPP_COMPILER_CLANG) &&                                       \
      (!defined(_LIBCPP_CLANG_VER) || _LIBCPP_CLANG_VER < 1100))
```

So I am now more confident in my suggestion that we need **either** `_LIBCPP_CLANG_VER_LESS_THAN` **or** simply to define `_LIBCPP_CLANG_VER` as `0` on Apple Clang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99515