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

[Aaron Jacobs via Phabricator via libcxx-commits]

jacobsa added inline comments.


================
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>;
-};
----------------
ldionne wrote:
> I think this would be an equivalent fix, no?
Perhaps, but is there any reason to prefer that? I don't see any benefit to the
two-level implementation; it just makes it harder to understand as far as I can
tell. Am I missing something?


================
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; };
----------------
ldionne wrote:
> Do we have the same issue for `disjunction`? Regardless, IMO we should add a test.
I added the corresponding tests there for symmetry. They do pass, although I'm
not confident there isn't a similar bug because its implementation is a little
weird. If you would take it, I'd be happy to send a follow-up commit to make
its implementation straightforward pattern matching like this one.


================
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 {};
----------------
ldionne wrote:
> 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?
> 
The instructions seem to assume that you've done some previous step that isn't
documented (or even linked?) there:

```
jacobsa at abel ~> cd clients/llvm-project/

jacobsa at abel ~/c/llvm-project> make check-cxx
make: *** No rule to make target 'check-cxx'.  Stop.

jacobsa at abel ~/c/llvm-project 2> cd build

jacobsa at abel ~/c/l/build> make check-cxx
make: *** No rule to make target 'check-cxx'.  Stop.
```

I'm sure how to deal with this is obvious to someone more experienced with the
project, but for me I couldn't figure it out. In contrast the other
instructions I linked work fine.


================
Comment at: libcxx/test/std/utilities/meta/meta.logical/conjunction.compile.pass.cpp:33
+struct CannotBeTwo {
+  static_assert(kVal != 2);
+};
----------------
ldionne wrote:
> 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);
> ```
Indeed, that's a much simpler reproducer, thanks. Switched to that.

I added my understanding of the reason it wasn't reproduced before to the
commit description.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136318