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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM w/ suggestion. If you don't have commit access and would like us to commit this for you, please provide `Author Name <email at domain>` for us to commit for you.

@philnik you can `arc patch` the diff and then `git commit --am --no-edit --author='Author Name <email at domain>'` to set the right author (sometimes `arc patch` will mess that up). Sorry if you already know that, I'm not sure you've committed stuff on behalf of others before but now you'll know :-).



================
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:
> philnik wrote:
> > 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.
> > I don't think that's a problem; I definitely didn't have it in mind then. I also can't remember why I implemented it in this way, most likely because I replaced `_And` and didn't think of just specializing `conjunction` itself.
> 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?
My thinking was along the lines of "what if a user specializes it, would it behave correctly"? That's extremely pedantic and I'm fine with your approach.

Thanks both for the explanation, I've been stretched for time to research myself.


================
Comment at: libcxx/test/std/utilities/meta/meta.logical/conjunction.compile.pass.cpp:83
 
+static_assert(!std::conjunction<std::false_type, HasNoValue, std::true_type>::value);
+static_assert(!std::conjunction_v<std::false_type, HasNoValue, std::true_type>);
----------------
I would suggest adding a comment linking to the bug report, otherwise one may be tempted to remove these seemingly not very useful tests in the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136318