This is an archive of the discontinued LLVM Phabricator instance.

[clang] Show error if defaulted comparions operator function is volatile or has ref-qualifier &&.
ClosedPublic

Authored by massberg on Apr 21 2023, 7:43 AM.

Details

Summary

This patch implemed the change proposed in [P2002R1] to 11.11.1 [class.compare.default] paragraph 1.

A defaulted compariosn operator function must be non-volatile and must either have no ref-qualifier or the ref-qualifier &.

Diff Detail

Event Timeline

massberg created this revision.Apr 21 2023, 7:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2023, 7:43 AM
massberg requested review of this revision.Apr 21 2023, 7:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2023, 7:43 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ilya-biryukov added a reviewer: Restricted Project.Apr 21 2023, 8:46 AM

Thanks! I believe we should also recover in the similar manner we do for missing const, see corresponding comment.

Extra NITs:

  • there is a typo in description: *compariosn* should be comparison,
  • we probably want to add this change to release notes.

Also adding @clang-language-wg in case someone else wants to chime in.

clang/include/clang/Basic/DiagnosticSemaKinds.td
9434

NIT: for consistency with wording of notes above.

9436

NIT: for consistency with the wording of err_ref_qualifier_constructor

clang/lib/Sema/SemaDeclCXX.cpp
8593–8601

NIT: this version is simpler and more aligned with the code below.
Also, could you move the volatile handling after the check for const?

Additionally, we seem to recover in case of const by adding it to the type and suggesting a fix-it to add it in the code.
Could we do the same for volatile and ref-qualifier, i.e. suggest a fix-it to remove the ref-qualifier and volatile and change the corresponding type to make AST pretend they were never there?

shafik added a subscriber: shafik.Apr 21 2023, 11:36 AM
shafik added inline comments.
clang/test/CXX/class/class.compare/class.compare.default/p1.cpp
18

Can we also have examples with out of line defaulting.

Precommit CI found a relevant failure that should be addressed.

clang/lib/Sema/SemaDeclCXX.cpp
8593
8593–8601

Note, you can shorten it further with:

return Diag(...);

because that will return true for you.

massberg updated this revision to Diff 517489.EditedApr 27 2023, 2:09 AM

Thanks for the comments!
Updated error messages, code and tests.

massberg marked 5 inline comments as done.Apr 27 2023, 2:18 AM
massberg added inline comments.
clang/lib/Sema/SemaDeclCXX.cpp
8593

Thanks! Which the change suggested by Ilya this has become obsolete.

8593–8601

Thanks, this is much simpler!

I have moved the code and added a recovery. However, adding a fix-it is much more complex as there is no simple way to get location to remove the keywords. Thus I decided to not offering a fix-it in this patch.

Basically looking good to me now, but the changes need a release note and I does this mean we can change cxx_status.html to claim full support for P2002R1 now or is there more left to be done for that? (If there's more left to do, adding those details to the cxx_status page would be appropriate.)

clang/include/clang/Basic/DiagnosticSemaKinds.td
9436

Still need to drop the s from operators

clang/lib/Sema/SemaDeclCXX.cpp
8593–8601

Yeah, it seems we don't track the ref qualifier location on any of the CXXMethodDecl, FunctionProtoType, or FunctionProtoTypeLoc classes. A future refactoring can consider adding locations for those to FunctionProtoTypeLoc, but needs to be able to handle multiple qualifiers (e.g., void func() const &; should track both the const and the & separately).

massberg updated this revision to Diff 519042.May 3 2023, 5:48 AM
massberg marked an inline comment as done.

Fixed typo, added release note and updated cxx_status.html.

massberg marked 3 inline comments as done.May 3 2023, 5:50 AM
aaron.ballman added a subscriber: rsmith.

Did you verify that the rest of P2002R1 was implemented correctly? There are a lot more changes in that paper than just this tiny bit, so if we're not certain this finishes things, we shouldn't mark the status as complete (but if we're figuring out what's missing, we should capture those details somewhere so we know what's left to do)

https://github.com/llvm/llvm-project/commit/b74a381296eef048911bb22dc4eb2d3598460470 is what marked the status as partial, but there's no explanation as to why it was partial. Perhaps @rsmith remembers what was left to be done there.

massberg updated this revision to Diff 519432.May 4 2023, 4:25 AM

Fix test that broke after last change.

I have checked the paper P2002R1 and as far as I can tell it is fully implemented when this patch has landed.

shafik added a comment.May 4 2023, 5:47 PM

I have checked the paper P2002R1 and as far as I can tell it is fully implemented when this patch has landed.

So I read the paper but since the paper does not have examples for each change nor does it have any links to DRs it is not obvious without spending some time to verify that we support each section. Changes like these really should come with examples in the paper to demonstrate behavior before and after.

I searched in the code and found that @rsmith labelled P2002R0 changes with comments and so a cursory look at those changes side by side with the paper seems to indicate that we probably covered most of the cases but I am not 100% sure. There is test coverage added with those changes as well but again relating each test to specific paper section is not trivial, at least for me.

The paper also uses the term constexpr compatible which has been replaced with constexpr suitable and not sure if that has any practical effect here.

ilya-biryukov added a comment.EditedMay 5 2023, 2:00 AM

The paper also uses the term constexpr compatible which has been replaced with constexpr suitable and not sure if that has any practical effect here.

They seem to be different terms, constexpr-suitable does not actually replace constexpr-compatible. The former refers to more basic checks (i.e. "not a coroutine", "not a ctor or dtor for a class with virtual bases"), the latter only pops up in context of defaulted functions.
Constexpr-compatible has been removed completely in P2448r2, but I think it's only applicable in C++23 and this change aims to implement C++20.

@massberg did we figure out if there is anything else left from P2002R1? Are there blockers to landing this?

@massberg did we figure out if there is anything else left from P2002R1? Are there blockers to landing this?

The only thing I came across was https://reviews.llvm.org/D151200 (error out when parameters of defaulted comparison operator aren't as expected), but this is already submitted.
Related to the defaulted comparison operators is DR 2450 (braced-init-list as a template-argument), but this isn't part of P2002R1.

So I'm not aware of any blockers.

ilya-biryukov accepted this revision as: ilya-biryukov.EditedJun 6 2023, 5:44 AM

Since there are no remaining blockers, LGTM from my side.
I suggest waiting for ~day to give chance to other reviewers.

@shafik, @aaron.ballman, @rsmith please let us know if you want to make another run through the paper to do extra checking.

This revision is now accepted and ready to land.Jun 6 2023, 5:44 AM

LGTM aside from a minor issue with the status page (we use unreleased until the current version is released).

clang/www/cxx_status.html
1008 ↗(On Diff #519432)
aaron.ballman accepted this revision.Jun 6 2023, 5:59 AM
shafik accepted this revision.Jun 6 2023, 4:59 PM

LGTM

massberg updated this revision to Diff 529216.Jun 7 2023, 1:55 AM

Fix status of paper in status page to unreleased.

massberg marked an inline comment as done.Jun 7 2023, 1:56 AM
massberg updated this revision to Diff 529231.Jun 7 2023, 3:23 AM

Format code.