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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
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.
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. |
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). |
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. |
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? |
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. |
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). |
Whoops, missed this comment. I'll be addressing your feedback and merging later this week.
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?
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 :)
Sorry, I had read @erichkeane's LGTM and taken that as actually approved. Would you like me to revert? I have reverted on principle.
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. | |
5515 | Could you put the implementation of BTT_IsConvertibleTo in its own function? |
clang/www/cxx_status.html | ||
---|---|---|
232 |
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) |
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. |
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 But if you prefer that can be done later! |
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.