Page MenuHomePhabricator

[clang] adds `__reference_constructs_from_temporary`
Needs ReviewPublic

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.

Depends on D135339.

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

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

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
1476

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
1476

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