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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik accepted this revision as: philnik.
philnik added a comment.

LGTM. Leaving final approval to @ldionne.



================
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.
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.


================
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:
> 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.
The `disjunction` implementation is a bit weird to avoid instantiating lots of classes. Unless you can show that the optimization doesn't actually do anything, I'd rather keep it as-is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136318