This is an archive of the discontinued LLVM Phabricator instance.

adds `__reference_constructs_from_temporary`
ClosedPublic

Authored by cjdb on Oct 5 2022, 7:16 PM.

Details

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.

Diff Detail

Event Timeline

cjdb created this revision.Oct 5 2022, 7:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2022, 7:16 PM
cjdb requested review of this revision.Oct 5 2022, 7:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2022, 7:16 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
erichkeane added a subscriber: Restricted Project.Oct 6 2022, 6:26 AM
cjdb planned changes to this revision.Oct 6 2022, 9:14 AM

I learnt last night that this one is incredibly wrong and needs revising.

ldionne added a subscriber: ldionne.Oct 6 2022, 1:34 PM

Interesting, I actually wasn't even aware of that C++23 feature in the library. FWIW, libc++ will be more than happy to use that builtin instead of trying to figure it out inside the library (if that's even possible)! We'll have to check whether GCC implements it, but hopefully it has something similar. So this definitely LGTM from the libc++ side of things, assuming the Clang folks are happy with the implementation.

cjdb added a comment.Oct 6 2022, 1:42 PM

Interesting, I actually wasn't even aware of that C++23 feature in the library. FWIW, libc++ will be more than happy to use that builtin instead of trying to figure it out inside the library (if that's even possible)! We'll have to check whether GCC implements it, but hopefully it has something similar. So this definitely LGTM from the libc++ side of things, assuming the Clang folks are happy with the implementation.

It's possible to easily implement reference_constructs_from_temporary in the library, but the converts one is far more tricky because __reference_binds_to_temporary only cares about construction. I don't know if GCC implements it (I thought it did, but I can't find the commit in a trivial Google search), but I did learn that libstdc++ has a guard for this name.

cjdb updated this revision to Diff 466088.Oct 7 2022, 8:50 AM

Gets __reference_constructs_from_temporary correct. Still WIP for the latter.

cjdb updated this revision to Diff 466318.Oct 8 2022, 4:49 PM
cjdb retitled this revision from [clang] adds `__reference_constructs_from_temporary` and `__reference_converts_from_temporary` to [clang] adds `__reference_constructs_from_temporary`.

discards work on __reference_converts_from_temporary for now. This feature isn't as trivial to implement as __reference_constructs_from_temporary, so it's deserving of its own commit. The two features are used exclusively, so it's not like adding one without the other will lead to an incomplete standard type.

erichkeane added inline comments.Oct 10 2022, 6:02 AM
clang/include/clang/Basic/DiagnosticCommonKinds.td
397 ↗(On Diff #466318)

I don't think we should diagnose this for individual identifiers, I don't think this adds much value, and the identifier is already reserved for implementation use. This implies we are going to diagnose ALL future reserved things, which we have no intention of doing.

cjdb added inline comments.Oct 10 2022, 9:53 AM
clang/include/clang/Basic/DiagnosticCommonKinds.td
397 ↗(On Diff #466318)

The motivation for this error is to ensure that standard library implementations don't take this name (__remove_cvref and __is_null_pointer had to get names that weren't a 1:1 mapping to their library counterparts as a result).

royjacobson added inline comments.
clang/docs/LanguageExtensions.rst
1623–1627

We should deprecate this, as __reference_constructs_from_temporary is implemented now. There's a list of deprecated builtins in SemaExprCXX.cpp:DiagnoseBuiltinDeprecation so I think it should be pretty easy to add a warning for users that this is a non-standard attribute.

royjacobson added inline comments.Oct 11 2022, 4:10 AM
clang/docs/LanguageExtensions.rst
1623–1627

Though for the CI's sake maybe it's better to wait until after libcxx stop using it.

I've been looking at implementing reference_constructs_from_temporary & friends and this would be sweet to have. Is this blocked on something specific?

clang/include/clang/Basic/DiagnosticCommonKinds.td
397 ↗(On Diff #466318)

Oh, just ask and we can rename ours! What do you want us to rename?

cjdb added a comment.Feb 7 2023, 8:26 PM

I've been looking at implementing reference_constructs_from_temporary & friends and this would be sweet to have. Is this blocked on something specific?

This trait should be ready to go, pending an LGTM. __reference_converts_from_temporary is a different story, and someone else might need to do this one if you need it in a timely fashion (but I'll be happy to review it).

clang/include/clang/Basic/DiagnosticCommonKinds.td
397 ↗(On Diff #466318)

Much appreciated! I think __remove_cvref had a libc++ conflict and __is_null_pointer had a libstdc++ conflict, so it's not specific to one library. I figured that it would be easier for backwards-compatibility reasons to just rename them to something slightly more awkward, so that previous versions of the stdlib don't spontaneously break for more recent compilers. This is more of an issue when you're dealing with package managers like apt, where some things depend on libc++-v$OLD, but there aren't any restrictions on Clang's version (for example, I use nightly Clang on Ubuntu, but apt notes that the Discord app has an older libc++ dependency).

After having let this sit for a few months, I don't really have strong opinions on reserving __std_trait anymore, so I'll touch this up in the morning.

I've been looking at implementing reference_constructs_from_temporary & friends and this would be sweet to have. Is this blocked on something specific?

This trait should be ready to go, pending an LGTM. __reference_converts_from_temporary is a different story, and someone else might need to do this one if you need it in a timely fashion (but I'll be happy to review it).

Sounds good! This naively looks good to me, but it would be nice if a Clang regular could take a look!

clang/include/clang/Basic/DiagnosticCommonKinds.td
397 ↗(On Diff #466318)

I just checked and I think we define __remove_cvref_t, but not __remove_cvref (that probably changed in the last year). In the future, don't hesitate to ping us if we are using a name that Clang would like to use, we should almost always be able to quickly get out of your way!

On the fence about the diagnostic at all, but definitely should not be doing string magic to make it quoted. Otherwise this is a LGTM.

clang/lib/Sema/SemaExprCXX.cpp
5715

This is my only problem with this... we shouldn't have to do this magic to get this to work. The diagnostic should have the quote in it, OR we should just pass an identifier (which, IIRC, ensures we wrap it in quotes anyway).

cjdb added a comment.Sep 11 2023, 2:24 PM

On the fence about the diagnostic at all, but definitely should not be doing string magic to make it quoted. Otherwise this is a LGTM.

Whoops, missed this comment. I'll be addressing your feedback and merging later this week.

cjdb updated this revision to Diff 556488.Sep 11 2023, 2:51 PM

responds to feedback:

  • removes diagnostic
  • adds documentation
cjdb marked 3 inline comments as done.Sep 11 2023, 2:52 PM

Is the changes to cxx_status.html still relevant/current? Also, did we ever figure out what GCC did for a builtin here? Do they have one yet? If not, have we encouraged them to implement this one? If so, DID they implement this one?

cjdb added a comment.Sep 11 2023, 3:43 PM

Is the changes to cxx_status.html still relevant/current?

Yeah.

Also, did we ever figure out what GCC did for a builtin here? Do they have one yet? If not, have we encouraged them to implement this one? If so, DID they implement this one?

They have implemented both, it seems :)

See https://godbolt.org/z/G8eeMfsej

cjdb added a comment.Sep 11 2023, 3:44 PM

Oh, I shouldn't have so much green in the status, whoops!

cjdb updated this revision to Diff 556497.Sep 11 2023, 3:47 PM
cjdb retitled this revision from [clang] adds `__reference_constructs_from_temporary` to adds `__reference_constructs_from_temporary`.
cjdb edited the summary of this revision. (Show Details)

Corrects cxx_status.html, which accidentally duplicated a lot of the file

cjdb updated this revision to Diff 556499.Sep 11 2023, 3:55 PM

I somehow made cxx_status.html worse in the previous commit, not better

This revision was not accepted when it landed; it landed in state Needs Review.Sep 11 2023, 4:14 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
cjdb added a comment.EditedSep 11 2023, 4:18 PM

Sorry, I had read @erichkeane's LGTM and taken that as actually approved. Would you like me to revert? I have reverted on principle.

cjdb reopened this revision.Sep 11 2023, 4:47 PM
cor3ntin added inline comments.Sep 12 2023, 12:42 AM
clang/lib/Sema/SemaExprCXX.cpp
33

Superfluous change

5513

BuiltinRemoveReference is nice here as I don't think we have a way to remove all cv and ref in one go at the ASTContext/QualType level.
However BuiltinAddPointer does nothing for us over Context.getPointerType(), so I'd rather we used that.

5515

Could you put the implementation of BTT_IsConvertibleTo in its own function?

All of @cor3ntin 's suggestions are really good ones, I'm ok with this once he is.

cjdb updated this revision to Diff 556609.Sep 12 2023, 1:46 PM
cjdb marked 3 inline comments as done.
  • responds to @cor3ntin's feedback
  • updates commit message to include [clang] tag and also indicate this is a revert
clang/lib/Sema/SemaExprCXX.cpp
33

Nice catch!

h-vetinari added inline comments.
clang/www/cxx_status.html
297
cor3ntin added inline comments.Sep 12 2023, 10:46 PM
clang/lib/Sema/SemaExprCXX.cpp
5515

I was thinking something like

QualType TPtr = S.Context.getPointerType(S.BuiltinRemoveReference(T, UnaryTransformType::RemoveCVRef, {}));
QualType UPtr = S.Context.getPointerType(S.BuiltinRemoveReference(U, UnaryTransformType::RemoveCVRef, {}));
return IsConvertibleTo(S, UPtr, TPtr);

and then EvaluateBinaryTypeTrait can use this new IsConvertibleTo (which we can extract from EvaluateBinaryTypeTrait)

cjdb added inline comments.Sep 13 2023, 9:27 AM
clang/lib/Sema/SemaExprCXX.cpp
5515

That sounds like an orthogonal change. How about I revert to what was previously there and do that as a second PR? It'll also need a location parameter.

cor3ntin added inline comments.Sep 13 2023, 10:19 AM
clang/lib/Sema/SemaExprCXX.cpp
5515

I don't insist. Please keep the getPointerType changes though.

It is pretty unusual to evaluate type traits in terms of the implementation of other type traits, because all of these things do different Sema (some of it is wasteful) and
could add undesired diagnostics in the future.

But if you prefer that can be done later!

cjdb updated this revision to Diff 556693.Sep 13 2023, 12:38 PM
cjdb marked an inline comment as done.

partially reverts change and fixes typo

This revision is now accepted and ready to land.Sep 13 2023, 12:47 PM
This revision was landed with ongoing or failed builds.Sep 13 2023, 4:52 PM
This revision was automatically updated to reflect the committed changes.