Page MenuHomePhabricator

[clang] adds unary type trait checks as compiler built-ins
AcceptedPublic

Authored by cjdb on Dec 25 2021, 11:23 AM.

Details

Reviewers
aaron.ballman
Summary

This is information that the compiler already has, and should be exposed
so that the library doesn't need to reimplement the exact same
functionality.

  • __is_nullptr
  • __is_bounded_array
  • __is_scoped_enum
  • __is_unbounded_array
  • __is_copy_constructible
  • __is_move_constructible
  • __is_copy_assignable
  • __is_move_assignable
  • __is_referenceable

Depends on D116203

Diff Detail

Event Timeline

cjdb created this revision.Dec 25 2021, 11:23 AM
cjdb requested review of this revision.Dec 25 2021, 11:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 25 2021, 11:23 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
cjdb added subscribers: EricWF, zoecarver.
cjdb updated this revision to Diff 396711.Dec 30 2021, 12:54 PM
cjdb edited the summary of this revision. (Show Details)

rebases to activate CI

cjdb updated this revision to Diff 396748.Dec 30 2021, 9:44 PM
cjdb edited the summary of this revision. (Show Details)

adds __is_unbounded_array

cjdb updated this revision to Diff 421836.Apr 10 2022, 11:17 PM
cjdb edited the summary of this revision. (Show Details)

adds __is_copy_constructible, __is_copy_assignable, __is_move_constructible (WIP), and __is_move_assignable (incomplete)

Herald added a project: Restricted Project. · View Herald TranscriptApr 10 2022, 11:17 PM
aaron.ballman added inline comments.Apr 11 2022, 5:39 AM
clang/lib/Sema/SemaExprCXX.cpp
4905–4906

Is a VLA a bounded array? (We support those as an extension in C++, unfortunately.)

5304

Why this dance and not Self.LangOpts.CPlusPlus? (Same below.)

5311

It looks like this file could stand to be clang formatted.

clang/test/SemaCXX/type-traits.cpp
736

Please add a test for VLAs to whichever function it should be tested under.

763

Any reason there's not a test with something like int[10] and int[]? (Same below.)

cjdb updated this revision to Diff 423280.Apr 16 2022, 11:17 PM
cjdb marked 3 inline comments as done.
  • finishes __is_move_constructible and __is_move_assignable
  • fixes detecting C++ mode
  • clang-formats
clang/lib/Sema/SemaExprCXX.cpp
4905–4906

__is_bounded_array(T) is true when "T is an array type of known bound". Do VLAs meet this criteria?

5311

Yeah, there was a time where arc diff did the formatting automatically, but it seems that day has passed :(

clang/test/SemaCXX/type-traits.cpp
763

Those are IntAr and IntArNB at the very top. I can rename those if you'd like.

aaron.ballman added inline comments.Apr 18 2022, 4:50 AM
clang/lib/Sema/SemaExprCXX.cpp
4905–4906

Heh, that's just it -- I don't know what constitutes "known". A VLA that tracks its extent "knows" the extent at runtime but possibly not compile time. So it's known-ish.

I think there's a few ways we could go about it.

  1. all VLAs are unbounded because some VLAs may not have their size known until runtime
  2. check the VLA's size expression and if we can find any way to constant evaluate it to a known value, then it's a bounded array
  3. make it an error to call this function with a VLA

My intuition is that #3 is the safest approach, but that #1 probably is sufficiently defensible as well. I worry a little bit that #2 will give surprising results without more investigation/effort (we sometimes fold VLAs into constant arrays, and I think users would expect this API to mirror that behavior).

clang/test/SemaCXX/type-traits.cpp
763

Oh, yeah, I totally missed that, thank you!

Up to you on the rename, so long as we have the coverage, that's the crucial bit.

cjdb added inline comments.Apr 18 2022, 10:43 AM
clang/test/SemaCXX/type-traits.cpp
763

Would you be against a patch where I found and replaced all TypeAr with Type[2] and TypeArNB with Type[]? I've done several double takes myself and question the readability of TypeAr.

aaron.ballman added inline comments.Apr 18 2022, 11:53 AM
clang/test/SemaCXX/type-traits.cpp
763

I'd be fine with that -- it would be an easy NFC change after this lands.

cjdb updated this revision to Diff 424672.Fri, Apr 22, 5:22 PM
cjdb marked 5 inline comments as done.
cjdb edited the summary of this revision. (Show Details)
  • handles VLAs
  • adds __is_referenceable which is used by portions of the standard library
cjdb updated this revision to Diff 424673.Fri, Apr 22, 5:25 PM

fixes formatting

aaron.ballman accepted this revision.Fri, Apr 29, 6:26 AM

LGTM modulo whatever changes are needed based on feedback from the original review (I noticed Richard has some suggestions there which may impact this review).

This revision is now accepted and ready to land.Fri, Apr 29, 6:26 AM
cjdb updated this revision to Diff 429410.Fri, May 13, 9:54 PM

makes __is_destructible and __is_nothrow_destructible KEYCXX instead of KEYMS (attention @aaron.ballman)

cjdb updated this revision to Diff 429479.Sat, May 14, 12:05 PM

rebases for D116203's sake