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

[Aaron Jacobs via Phabricator via libcxx-commits]

jacobsa marked 5 inline comments as done.
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:
> 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.
It seems like [meta.logical]/3 specifically says it *will* be a specialization,
if I read the wording correctly:

    https://timsong-cpp.github.io/cppwp/n4868/meta.logical#3

> For a specialization `conjunction<B1, …, BN>`, if there is a ...

That said, is there even any way to differentiate the two from outside?


================
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>
----------------
ldionne wrote:
> 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.
Thanks, sorry for getting this wrong. I'm quite confused by the fact that there
are threads with replies, but replies are individually marked done. If I reply
to a comment with a question or a follow-up, am I supposed to mark the original
as done? Am I supposed to mark my own comment as done (the default)? It's kind
of weird coming from Google's code review system, where the done bit applies to
entire threads.


================
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:
> 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.
My point is that those instructions don't discuss any of that. The section
mentions a bootstrapping build, but doesn't define the terms. It doesn't say
anything about using cmake first; it implies all you have to do is run `make
check-cxx`.

I'm not saying this is wrong, but it's definitely incomplete. It was not
sufficient for me to "get started". Does that make sense?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136318