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

[Aaron Jacobs via Phabricator via libcxx-commits]

jacobsa added a comment.

Thanks! I don't have commit access, so it would be great if you could do it.
Please credit:

  Aaron Jacobs <jacobsa at google.com>



================
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:
> > 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.
Ah I see, thanks. Programs that specialize std::conjunction are forbidden [according to cppreference](https://en.cppreference.com/w/cpp/language/extending_std):

> It is allowed to add template specializations for any standard library class template to the namespace `std` only if the declaration depends on at least one program-defined type [...]
>
> [..]
>
> * None of the templates defined in `<type_traits>` may be specialized for a program-defined type, except for `std::common_type` and `std::basic_common_reference`. This includes the type traits and the class template std::integral_constant.

I believe this comes from [meta.rqmts/4](https://timsong-cpp.github.io/cppwp/n4868/meta#rqmts-4) in C++20:

> Unless otherwise specified, the behavior of a program that adds specializations for any of the templates specified in [meta] is undefined.


================
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; };
----------------
philnik wrote:
> 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.
Ack, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136318