[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:187
+#  ifdef __apple_build_version__
+#    define _LIBCPP_CLANG_VER 0
+#  else
----------------
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.


================
Comment at: libcxx/include/future:505
+#if _LIBCPP_STD_VER > 14
+    // explicit future_error(future_errc _Ev) : logic_error(), __ec_(make_error_code(_Ev)) {}
 #endif
----------------
ldionne wrote:
> curdeius wrote:
> > Quuxplusone wrote:
> > > curdeius wrote:
> > > > This was already dead code, so just commented it out in this patch.
> > > > I'll create another patch to fix this if that's wanted. However, the `future_error(error_code __ec);` gets called with arguments of type `future_errc`, so adding `future_error(future_errc)` ctor as `future_error(future_errc _Ev) : future_error(make_error_code(_Ev) {}` will not add anything.
> > > > We might however want to do something more standard conformant (mind that `future(error_code)` ctor is "exposition only").
> > > I recommend just deleting lines 504–506. That clearly preserves the existing behavior (which is not buggy AFAWCT, right?). If the existing behavior //is// buggy, then you should add a test that detects the bug.
> > > I would not check in commented-out code.
> > The existing behaviour is correct but not very strict (it accepts what standard prescribes, but also more).
> > Precisely:
> > * the ctor is not explicit
> > * the ctor accepts not only `future_errc` (which is indirect through `error_code`), but also `error_code`
> > That's seems acceptable though (because changing that will break some code depending on this).
> > Anyway, it's out of scope of this patch IMO.
> I'd welcome a patch that makes the constructors strictly standard conforming. Agreed this belongs to a different patch though.
D99567.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99515