[libcxx-commits] [PATCH] D136318: [libc++] type_traits: fix short-circuiting in std::conjunction.

[Louis Dionne via Phabricator via libcxx-commits]

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

Thanks for the fix! Some comments but generally looks like this is right.



================
Comment at: libcxx/include/__type_traits/conjunction.h:26-28
-struct __conjunction_impl {
-  using type = conditional_t<!bool(_Arg::value), _Arg, typename __conjunction_impl<_Args...>::type>;
-};
----------------
I think this would be an equivalent fix, no?


================
Comment at: libcxx/test/std/utilities/meta/meta.logical/conjunction.compile.pass.cpp:23
 struct False { static constexpr bool value = false; };
 
 struct MySpecialTrueType { static constexpr auto value = true; static constexpr auto MySpecial = 23; };
----------------
Do we have the same issue for `disjunction`? Regardless, IMO we should add a test.


================
Comment at: libcxx/test/std/utilities/meta/meta.logical/conjunction.compile.pass.cpp:26
 struct MyOtherSpecialTrueType { static constexpr auto value = -1; static constexpr auto MySpecial = 46; };
 struct MySpecialFalseType { static constexpr auto value = false; static constexpr auto MySpecial = 37; };
 struct HasNoValue {};
----------------
Regarding your comment on the Github issue:

> Reminding my future self to look here for instructions to build and test libc++, because the getting started instructions don't work, or at least don't document the prerequisite steps. 

What didn't work with the instructions at https://libcxx.llvm.org/TestingLibcxx.html#getting-started?



================
Comment at: libcxx/test/std/utilities/meta/meta.logical/conjunction.compile.pass.cpp:33
+struct CannotBeTwo {
+  static_assert(kVal != 2);
+};
----------------
Can you explain why our existing test case for `HasNoValue` doesn't cover this? Is it possible that simply adding the following test case would be sufficient?

```
static_assert(!std::conjunction<std::false_type, HasNoValue, std::true_type>::value);
```


================
Comment at: libcxx/test/std/utilities/meta/meta.logical/conjunction.compile.pass.cpp:94
+
+// Regression test for issue 58490 (short-circuiting with static assertions).
+static_assert(!std::conjunction<std::false_type, CannotBeTwo<2>>::value);
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136318