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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

So with the proposed changes, we'd have:

  _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 makes sense. @curdeius @Quuxplusone I'm curious to hear if you think that makes sense too, but if so let's move forward with that. Perhaps you could also add a comment where `_LIBCPP_COMPILER_CLANG_BASED` is first defined to explain what I wrote above?



================
Comment at: libcxx/include/__config:187
+#  ifdef __apple_build_version__
+#    define _LIBCPP_CLANG_VER 0
+#  else
----------------
curdeius wrote:
> ldionne wrote:
> > I fear this will have the consequence to disable any feature guarded by `#if _LIBCPP_CLANG_VER > XXXX` on AppleClang, even if the compiler is perfectly able to handle that feature.
> > 
> > Edit: I looked and this doesn't seem to be an issue, but I would still suggest the following:
> > 
> > ```
> > #if defined(__clang__) && !defined(__apple_build_version__)
> > #  define _LIBCPP_COMPILER_CLANG_BASED
> > #  define _LIBCPP_CLANG_VER (__clang_major__ * 100 + __clang_minor__)
> > #elif defined(__apple_build_version__)
> > #  define _LIBCPP_COMPILER_CLANG_BASED
> > #  define _LIBCPP_APPLE_CLANG_VER (__clang_major__ * 100) + __clang_minor__
> > #elif ...
> > ```
> > 
> > Notice the rename of `_LIBCPP_COMPILER_CLANG` to `_LIBCPP_COMPILER_CLANG_BASED`. In the few places where we currently use `_LIBCPP_COMPILER_CLANG`, it seems like what we really want to ask is whether the compiler is Clang-like and supports Clang-like features, which is the case on both LLVM Clang and Apple Clang.
> > 
> > Introducing a different macro `_LIBCPP_APPLE_CLANG_VER` also makes it clear that we consider it a different compiler.
> Should we really `#  define _LIBCPP_APPLE_CLANG_VER (__clang_major__ * 100) + __clang_minor__`?
> 
> Does `__clang_minor__` includes the patch number in "10.0.1"?
> In the cases where this version is taken into account, we look at the patch number as well (checking `__apple_build_version__` directly). E.g.:
> ```
> // Literal operators ""d and ""y are supported starting with LLVM Clang 8 and AppleClang 10.0.1
> #if (defined(_LIBCPP_CLANG_VER) && _LIBCPP_CLANG_VER < 800) ||                 \
>     (defined(__apple_build_version__) && __apple_build_version__ < 10010000)
> #define _LIBCPP_HAS_NO_CXX20_CHRONO_LITERALS
> #endif
> ```
> 
> Shouldn't it be something like: `#  define _LIBCPP_APPLE_CLANG_VER (__apple_build_version__ / 10000)`? I.e. just removing 4 trailing zeroes (e.g. 10010000 -> 1001).
> 
> Note: I have no access to apple clang.
My definition of `_LIBCPP_APPLE_CLANG_VER` was indeed buggy. I agree with `_LIBCPP_APPLE_CLANG_VER == (__apple_build_version__ / 10000)`. This way, for example AppleClang 12.0.5 would give `__apple_build_version__ == 12050019`, so `_LIBCPP_APPLE_CLANG_VER == 1205`, which is quite natural.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99515