Page MenuHomePhabricator

[clang] [docs] Update the changes of C++20 Modules in clang15
ClosedPublic

Authored by ChuanqiXu on Jul 5 2022, 4:36 AM.

Details

Summary

Since clang15 is going to be branched in July 26, and C++ modules still lack an update on ReleaseNotes. Although it is not complete yet, I think it would be better to add one since we've done many works for C++20 Modules in clang15.

@iains would you like to check if the list is complete or not? I don't include D126694 since I feel like it might not be possible to be landed in 3 weeks. I know there some other patches not covered, but I feel it might be noise to our users.

Diff Detail

Event Timeline

ChuanqiXu created this revision.Jul 5 2022, 4:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2022, 4:36 AM
ChuanqiXu requested review of this revision.Jul 5 2022, 4:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2022, 4:36 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ChuanqiXu edited the summary of this revision. (Show Details)Jul 5 2022, 4:41 AM
ChuanqiXu edited the summary of this revision. (Show Details)

Wording looks fine to me, though unsure if this is a complete/accurate list for obvious reasons. If @iains can take a quick look, it woudl be greatly appreciated.

iains added a comment.Jul 6 2022, 1:40 AM

Perhaps we could be a little more bold about the completeness of the implementation (at least, w.r.t the base paper P1103) - we pass the relevant test cases.

As for the follow-on papers, I think we have more that can be added notes below:
There are some test cases to be posted to phab for some of these (so maybe allow me a few more days to get the list fully correct).

(it will also depend on what we can land before 26th - however some of the stuff below is already approved, so it's a matter of finding some time to push the patches and watch the bots...)

@ChuanqiXu if you think that more is needed on any of these (other than P1815 which is known partial), please let me know.

(Please also add any relevant phab reviews from your side)


P1779R3: ABI isolation for member functions

  • Paper applied to WP.

Change [dcl.inline]/7 (as edited by P1815R1):

  • This is being addressed by D128328

(although we have somewhat of a moving target since some clarifications were requested from core).

Change [dcl.fct.def.default]/3:

  • Already handled in the constexpr/consteval code, there is no actual change for modular cases.

Change [class.mfct]/1:
Change [class.friend]/7:


P1979R0: Resolution to US086

  • Paper applied to WP.
  • Current implementation complies: test case to be posted CXX/module/module.import/p7.cpp

P1811R0 Relaxing redefinition restrictions for re-exportation robustness

  • Paper applied to WP - note there are on-going discussions in core/ext about exports that might affect this.

Note that there are a lot of changes here, but that the draft that includes them is what we are working to, so it is expected that (mostly) we will need to identify tests and/or queries about how to verify.

Change in 6.2 [basic.def.odr] paragraph 1:
Change in 6.2 [basic.def.odr] paragraph 12:

  • no action required, these changes restore pre-P1103 behaviour (and the paragraph 12 changes have been subsumed in following updates).

Change in 10.5 [module.context] paragraph 7:

test: CXX/module/module.context/p7.cpp

Change in 10.6 [module.reach] paragraph 3:

Change in 15.2 [cpp.include] paragraph 7:

  • D128981 provides conditional include translation that follows the same scheme as GCC.

Feature test macro

  • already implemented.

[Partial] P1815R2: Translation-unit-local entities

Change [basic.def.odr]/10:

  • implementation complies, example provided.

Insert before [basic.def.odr]/11

  • D128328 + ongoing core/ext discussions.

Change [basic.lookup.argdep]/5:
D129174 - overload changes. example added
Question to core on TU#3 f(x).


P2115R0: US069: Merging of multiple definitions for unnamed unscoped enumerations

  • merged to WP, but ongoing discussions in core/ext might cause re-work

testcase to be posted to phab.

ChuanqiXu updated this revision to Diff 442487.Jul 6 2022, 2:45 AM

Address comments.

Perhaps we could be a little more bold about the completeness of the implementation (at least, w.r.t the base paper P1103) - we pass the relevant test cases.

I added the wording like Implemented P1103R3`. I am not sure if it is good to say this. We made some progress indeed. But both of us know there are some FIXME remains.

As for the follow-on papers, I think we have more that can be added notes below:
There are some test cases to be posted to phab for some of these (so maybe allow me a few more days to get the list fully correct).

(it will also depend on what we can land before 26th - however some of the stuff below is already approved, so it's a matter of finding some time to push the patches and watch the bots...)

@ChuanqiXu if you think that more is needed on any of these (other than P1815 which is known partial), please let me know.

(Please also add any relevant phab reviews from your side)


P1779R3: ABI isolation for member functions

  • Paper applied to WP.

Change [dcl.inline]/7 (as edited by P1815R1):

  • This is being addressed by D128328

(although we have somewhat of a moving target since some clarifications were requested from core).

Change [dcl.fct.def.default]/3:

  • Already handled in the constexpr/consteval code, there is no actual change for modular cases.

Change [class.mfct]/1:
Change [class.friend]/7:


P1979R0: Resolution to US086

  • Paper applied to WP.
  • Current implementation complies: test case to be posted CXX/module/module.import/p7.cpp

P1811R0 Relaxing redefinition restrictions for re-exportation robustness

  • Paper applied to WP - note there are on-going discussions in core/ext about exports that might affect this.

Note that there are a lot of changes here, but that the draft that includes them is what we are working to, so it is expected that (mostly) we will need to identify tests and/or queries about how to verify.

Change in 6.2 [basic.def.odr] paragraph 1:
Change in 6.2 [basic.def.odr] paragraph 12:

  • no action required, these changes restore pre-P1103 behaviour (and the paragraph 12 changes have been subsumed in following updates).

Change in 10.5 [module.context] paragraph 7:

test: CXX/module/module.context/p7.cpp

Change in 10.6 [module.reach] paragraph 3:

Change in 15.2 [cpp.include] paragraph 7:

  • D128981 provides conditional include translation that follows the same scheme as GCC.

Feature test macro

  • already implemented.

[Partial] P1815R2: Translation-unit-local entities

Change [basic.def.odr]/10:

  • implementation complies, example provided.

Insert before [basic.def.odr]/11

  • D128328 + ongoing core/ext discussions.

Change [basic.lookup.argdep]/5:
D129174 - overload changes. example added
Question to core on TU#3 f(x).


P2115R0: US069: Merging of multiple definitions for unnamed unscoped enumerations

  • merged to WP, but ongoing discussions in core/ext might cause re-work

testcase to be posted to phab.

Thanks for the summary! But I am afraid it is too fine-grained to users. It looks like the current ReleaseNotes don't contain phab links nor new added test cases. @erichkeane @aaron.ballman may you give some advice?

iains added a comment.Jul 6 2022, 2:57 AM

I would not expect to add all this information to the release notes, or any of the phab links - just single lines to say that paper numbers are implemented

  • the details are just to help us track the situation up to 26th July.
h-vetinari added inline comments.
ChuanqiXu updated this revision to Diff 442761.Jul 6 2022, 8:10 PM

Update cxx_status.html

ChuanqiXu marked an inline comment as done.Jul 6 2022, 8:10 PM

I would not expect to add all this information to the release notes, or any of the phab links - just single lines to say that paper numbers are implemented

  • the details are just to help us track the situation up to 26th July.

Got it. Thanks!

clang/docs/ReleaseNotes.rst
472–476

Done. Thanks!

avogelsgesang added inline comments.
clang/www/cxx_status.html
1183 ↗(On Diff #442761)

should this be class="unreleased" instead of class="full"? At least this is what other places in this document use when mentioning clang 15

ChuanqiXu updated this revision to Diff 443549.Jul 10 2022, 8:26 PM
ChuanqiXu marked an inline comment as done.

Address comments.

clang/www/cxx_status.html
1183 ↗(On Diff #442761)

Oh, you're right. Thanks for reporting!

@iains I'm going to land this in next Monday if all the dependent patch landed. Do you feel good with this?

iains added a comment.Jul 24 2022, 3:14 PM

@iains I'm going to land this in next Monday if all the dependent patch landed. Do you feel good with this?

I am not sure if we will get p1815 part 1 landed (effectively today) - and, of course, we have also some other patches in-flight that will improve the C++20 implementation, but extremely unlikely to be landed today.

ChuanqiXu accepted this revision.Jul 26 2022, 3:46 AM

@iains I'm going to land this in next Monday if all the dependent patch landed. Do you feel good with this?

I am not sure if we will get p1815 part 1 landed (effectively today) - and, of course, we have also some other patches in-flight that will improve the C++20 implementation, but extremely unlikely to be landed today.

Now it is landed, I think we could land the ReleaseNotes too.

This revision is now accepted and ready to land.Jul 26 2022, 3:46 AM