cjdb added inline comments.
================
Comment at: clang/include/clang/Basic/TokenKinds.def:528
+TYPE_TRAIT_1(__is_nothrow_copy_constructible, IsNothrowCopyConstructible,
KEYCXX)
+TYPE_TRAIT_1(__is_trivially_copy_constructible, IsTriviallyCopyConstructible,
KEYCXX)
TYPE_TRAIT_2(__reference_binds_to_temporary, ReferenceBindsToTemporary, KEYCXX)
----------------
jwakely wrote:
> cjdb wrote:
> > ldionne wrote:
> > > cjdb wrote:
> > > > erichkeane wrote:
> > > > > royjacobson wrote:
> > > > > > erichkeane wrote:
> > > > > > > cjdb wrote:
> > > > > > > > erichkeane wrote:
> > > > > > > > > So this one is a whole 'thing'. The Clang definition of
> > > > > > > > > 'trivially copy constructible' is a few DRs behind. We
> > > > > > > > > should probably discuss this with libcxx to make sure use of
> > > > > > > > > this wouldn't be broken.
> > > > > > > > I'd prefer to get those DRs in before finalising D135238 and
> > > > > > > > subsequent ones. Do you know the DR numbers I should be looking
> > > > > > > > at, or should I just poke npaperbot?
> > > > > > > Not off the top of my head, Aaron and I both poked at it at one
> > > > > > > point trying to get trivially constructible right at one point,
> > > > > > > but I think we both gave up due to the ABI/versioning concerns.
> > > > > > Maybe DR1734? Although it's about the trivially copyable trait, not
> > > > > > trivially copy constructible.
> > > > > >
> > > > > Yeah, I think that was the DR, that number sounds familiar.
> > > > The `__is_trivially_*` traits were, in part, what inspired the Great
> > > > Split of D116208. I could remove them for now and revisit once I rip my
> > > > hair out over these DRs, if that would substantially improve the
> > > > chances of these commits landing (other commentary notwithstanding).
> > > I am not sure I see a problem with the "triviality" part of this -- we
> > > already use a compiler builtin for `std::is_trivially_constructible`, so
> > > I would expect either this patch is fine, or we already have a latent bug
> > > in libc++.
> > >
> > > I think I can echo @philnik's comment about this not necessarily
> > > providing the biggest benefit since our implementation of
> > > `std::is_trivially_copy_constructible` is a fairly trivial wrapper on top
> > > of `__is_trivially_constructible`, but I wouldn't object to the patch on
> > > that basis. I think it would probably be possible to instead provide a
> > > set of basis builtin operations that we can then build all of the library
> > > type traits on top of -- that would provide the highest bang-for-our-buck
> > > ratio.
> > >
> > > At the same time, there's something kind of enticing in the consistency
> > > of defining every single type trait as a builtin, without exception. If
> > > that's the end goal, I think that would also be neat and we'd likely
> > > regroup all of our type traits under a single header, since each of them
> > > would literally be a one liner.
> > >
> > > There's also the question of whether GCC provides these builtins -- if
> > > they don't and if they don't have plans to, then we'd actually need to
> > > add complexity in libc++ to support both, which we would be unlikely to
> > > do given that there's probably not a huge compile-time performance
> > > benefit.
> > >
> > > TLDR, I think the two questions that can help gauge how much interest
> > > there will be from libc++ to use this are:
> > >
> > > 1. Is the plan to provide *all* the type traits as builtins?
> > > 2. Will GCC implement them?
> > >
> > > That being said, libc++ might not be the only potential user of these
> > > builtins, so I wouldn't necessarily make it a hard requirement to satisfy
> > > us.
> > >
> > > I think I can echo @philnik's comment about this not necessarily
> > > providing the biggest benefit since our implementation of
> > > `std::is_trivially_copy_constructible` is a fairly trivial wrapper on top
> > > of `__is_trivially_constructible`, but I wouldn't object to the patch on
> > > that basis.
> >
> > I haven't had time to do anything properly in the way of benchmarking, but
> > after looking at @philnik's quoted code, I see that I'd naively addressed
> > `__is_constructible(T, T const&)`, forgetting that `__add_lvalue_reference`
> > would've fixed that issue.
> >
> > > 1. Is the plan to provide *all* the type traits as builtins?
> >
> > Yes, with the possible exception of `enable_if` and `add_const` etc. (see
> > D116203 for why the qualifier ones aren't already in). The hardest ones
> > will probably be `common_type`, `common_reference`, `*invocable*`, and
> > `*swappable*`. The former two depend on technology that doesn't exist in
> > Clang yet, and the latter two are likely hard due there not being prior art.
> >
> > > 2. Will GCC implement them?
> >
> > @jwakely do you know if there can be cross-compiler synergy here?
> >
> >
> Which traits are you asking about, just the
> __is_{,trivially,nothrow}_copy_constructible ones? Or all type traits?
>
> Either way, I think the answer is no. We already use
> __is_{,trivially,nothrow}_constructible for the copy-constructible traits,
> and it works fine. I'm not aware of any problems with it. So I don't see any
> benefit in adding these ones. As already mentioned, it creates *more* code to
> maintain when using these traits, because we still have to support older
> versions of other compilers that don't have all the new traits yet.
>
> We've added traits for a few more things recently (`__remove_cv`,
> `__remove_cvref`, `__remove_reference`) but they can only be used in limited
> ways (you can't use them directly to implement `std::remove_cv_t` for
> example, because they don't mangle to the same thing, which is observable).
> So I'm sceptical about the benefits of implementing *every* trait as a
> built-in. There are some which are bottlenecks and worth doing, and some
> which aren't.
> Which traits are you asking about, just the
> __is_{,trivially,nothrow}_copy_constructible ones? Or all type traits?
>
> Either way, I think the answer is no. We already use
> __is_{,trivially,nothrow}_constructible for the copy-constructible traits,
> and it works fine. I'm not aware of any problems with it. So I don't see any
> benefit in adding these ones. As already mentioned, it creates *more* code to
> maintain when using these traits, because we still have to support older
> versions of other compilers that don't have all the new traits yet.
>
Thanks for filling in the knowledge-gap here. I think libc++ only supports the
most recent two releases of Clang (and I think only the latest release of GCC),
so that library code would have an expiration date of around most eighteen
months. So I'm clearer on the libstdc++ side of things, how far back do you
normally support?
> We've added traits for a few more things recently (`__remove_cv`,
> `__remove_cvref`, `__remove_reference`) but they can only be used in limited
> ways (you can't use them directly to implement `std::remove_cv_t` for
> example, because they don't mangle to the same thing, which is observable).
Interesting. Is this a libstdc++ backwards-compat issue, or does it burrow into
the Itanium ABI, which has mangling conventions for builtin type transformers?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135238/new/
https://reviews.llvm.org/D135238
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
