[PATCH] D135238: [clang] adds copy-constructible type-trait builtins

Thu Oct 6 13:49:18 PDT 2022

ldionne 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)
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135238