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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Requesting changes because of the `_LIBCPP_COMPILER_CLANG` comment, but this LGTM overall.



================
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
----------------
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.


================
Comment at: libcxx/include/type_traits:840
+    !(defined(_LIBCPP_COMPILER_CLANG) && defined(_LIBCPP_CLANG_VER) &&         \
+      _LIBCPP_CLANG_VER < 1100)
 
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99515