[libcxx-commits] [PATCH] D125221: [libc++] Fix conjunction/disjunction and mark a few LWG issues as complete

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__type_traits/conjunction.h:26
+struct __conjunction_impl {
+  using __derive_from = conditional_t<!bool(_Arg::value), _Arg, typename __conjunction_impl<_Args...>::__derive_from>;
+};
----------------
It would be more idiomatic to use `using type = ...;` here and below.


================
Comment at: libcxx/include/type_traits:481-487
 template <class ...> using __expand_to_true = true_type;
 template <class ..._Pred>
 __expand_to_true<__enable_if_t<_Pred::value>...> __and_helper(int);
 template <class ...>
 false_type __and_helper(...);
 template <class ..._Pred>
 using _And _LIBCPP_NODEBUG = decltype(__and_helper<_Pred...>(0));
----------------
I would consider moving these helpers to `conjunction.h` and `disjunction.h` in the future, with a small comment explaining how they differ from `disjunction` and `conjunction`.


================
Comment at: libcxx/test/std/utilities/meta/meta.logical/conjunction.compile.pass.cpp:7
+//
+//===----------------------------------------------------------------------===//
+
----------------
Can you please make sure that we have coverage for all the cases that were reported in https://github.com/llvm/llvm-project/issues/54803? The bug report has some nice tests we can easily add to our test suite in one form or another.


================
Comment at: libcxx/test/std/utilities/meta/meta.logical/conjunction.compile.pass.cpp:9
+
+// UNSUPPORTED: c++03, c++11, c++14
+// type_traits
----------------
>From live review: let's also make sure we allow types that have only an explicit cast to `bool`, since the standard says `bool(Bi::value)` (https://eel.is/c++draft/meta.logical).


================
Comment at: libcxx/test/std/utilities/meta/meta.logical/conjunction.compile.pass.cpp:76
+
+static_assert(std::is_base_of_v<std::false_type, std::conjunction<std::false_type, HasNoValue>>);
----------------
Please add something like

```
static_assert(!std::conjunction<False, HasNoValue, True>::value);
```

We need to make sure that we can instantiate it, not only use `is_base_of` on it (yes, I know `is_base_of` will instantiate it, but that's only how our implementation works). Same applies for disjunction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125221