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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a subscriber: philnik.
ldionne 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>;
-};
----------------
jacobsa wrote:
> 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?
I believe the goal might have been to avoid partially specializing `std::conjunction` itself as a matter of conformance. @philnik is that something you had in mind? If so, we'd need a test for that too (which @philnik can help you with since he should have added it when he last modified these traits :-) ).

If that's not a concern, then either implementation should do.


================
Comment at: libcxx/test/std/utilities/meta/meta.logical/conjunction.compile.pass.cpp:15
 //   constexpr bool conjunction_v = conjunction<B...>::value;         // C++17
 
 #include <cassert>
----------------
General tip: you can mark review comments as done when you've addressed them, we generally use that to track what's still left to address on a review! In this case I'll mark this very comment as done since I'm only putting this here for your information, with no action item associated to it.


================
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; };
----------------
jacobsa wrote:
> 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.
That may make sense, but let's first hear what @philnik has to say about the implementation of conjunction.


================
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 {};
----------------
jacobsa wrote:
> 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.
Is it possible that you are doing a bootstrapping build, and that the Note item on that page applies to you? If not, I'd be curious to see what your CMake command looks like.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136318