This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Updating handling of defaulted comparison operators to reflect changes from P2448R2
ClosedPublic

Authored by shafik on Mar 14 2023, 1:52 PM.

Details

Summary

Prior to P2448R2 we were more aggressive in diagnosing ill-formed constexpr functions. Many of these restrictions were relaxed and now it is not required for defaulted comparison operators to call constexpr functions.

This behavior is extended to before C++23 and diagnostic for it's use can be enabled w/ -pedantic or -Wc++2b-default-comp-relaxed-constexpr

This fixes: https://github.com/llvm/llvm-project/issues/61238

Diff Detail

Event Timeline

shafik created this revision.Mar 14 2023, 1:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2023, 1:52 PM
shafik requested review of this revision.Mar 14 2023, 1:52 PM

Please also update the P2448 row in cxx_status.html and add release notes.

clang/include/clang/Basic/DiagnosticSemaKinds.td
9429–9444

The grammar of these diagnostics looks a bit peculiar. Suggested an alternative, but it's still a bit awkward ("...but for which..."), maybe you can find something better.

9432

We usually spell these CXXabCompat diagnostics warn_cxxab_compat_...

clang/lib/Sema/SemaDeclCXX.cpp
8808–8826

Would be good to mention the change in C++23 here.

8828–8829

We'll also be getting diagnostics here if the parameter and return types are not literal types. You can pass ...::CheckValid instead of ...::Diagnose to find out if there would be an error, but we should probably instead make the CheckConstexpr... functions produce warnings (ExtWarn prior to C++23) rather than errors in general. That's a much larger change, though -- there's probably half a dozen diagnostics in the CheckConstexpr* functions that will need to be updated to properly support P2448R2. If you just want to implement this one part of P2448 for now and leave the rest for a later change, that seems fine to me.

8833

Don't need the parens around this. This layout is a little unusual, what does clang-format do here?

clang/test/CXX/class/class.compare/class.compare.default/p3.cpp
134

Can we test these both ways, like you did in p4?

shafik updated this revision to Diff 505985.Mar 16 2023, 9:37 PM
shafik marked 6 inline comments as done.
  • Updating diagnostic wording
  • Adding Reference to changes from p2448r2 to the comments
  • Modified p3 tests to have an extension pass as well
shafik updated this revision to Diff 505986.Mar 16 2023, 9:47 PM
  • Added release note
  • Update CXX status
shafik added inline comments.Mar 16 2023, 9:48 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
9429–9444

@aaron.ballman any suggestions for less awkward wording here? I don't think what there is here now is too bad but asking for a another opinion.

clang/lib/Sema/SemaDeclCXX.cpp
8828–8829

I don't think I have the bandwidth for a larger change but I can file a bug and assign myself to it and I will put in my work queue.

8833

My bad, I forgot to apply clang-format.

gentle ping

aaron.ballman added inline comments.Apr 5 2023, 9:34 AM
clang/docs/ReleaseNotes.rst
103–104

Should we also say what's still missing that causes this to be partial?

clang/include/clang/Basic/DiagnosticSemaKinds.td
9429–9444

Yeah, that wording is a bit... wordy, but I'm struggling to think of something more approachable that's still accurate.

9434–9440

Formatting fix-up and a wording correction.

clang/lib/Sema/SemaDeclCXX.cpp
8819
clang/test/CXX/class/class.compare/class.compare.default/p4.cpp
153

no diagnostic extension-warning?

162

Can you add an instantiation that does use a constexpr suitable type to show that we don't issue the diagnostic on that instantiation, or do you think that is sufficiently covered by other test coverage?

clang/www/cxx_status.html
1483–1490

Please add <details> markup to explain why the support is partial, and it's probably not a bad idea to say Clang 17 (Partial) so users know when the partial support started.

I'm happy whenever Aaron is.

clang/lib/Sema/SemaDeclCXX.cpp
8817
8835–8836

I wonder if incorrect_ is the best way to spell the diagnostic ID given the new rule. Suggested an alternative.

shafik updated this revision to Diff 513397.Apr 13 2023, 5:38 PM
shafik marked 8 inline comments as done.
  • Updating diagnostic wording, formatting and names
  • Updated release notes
  • Updated c++ status
  • Updates tests to comment that they will change once we fully support P2448R2
  • Various minor fixes
clang/test/CXX/class/class.compare/class.compare.default/p4.cpp
162

I think I need to add more tests, or add comments to existing tests wrt to p2448

shafik updated this revision to Diff 513412.Apr 13 2023, 7:29 PM

Fix DiagnosticSemaKinds.td , was missing an opening quote

aaron.ballman accepted this revision.May 3 2023, 12:44 PM

Just some minor nits from me, but LG with those fixed up.

clang/include/clang/Basic/DiagnosticGroups.td
230–232 ↗(On Diff #513412)
clang/include/clang/Basic/DiagnosticSemaKinds.td
9435

Note, you should coordinate the 2b->23 changes with https://reviews.llvm.org/D149553

clang/www/cxx_status.html
1486

Indentation looks a bit off here.

This revision is now accepted and ready to land.May 3 2023, 12:44 PM
shafik updated this revision to Diff 519555.May 4 2023, 10:25 AM
shafik marked 4 inline comments as done.
  • Removed diagnostic group
  • fixed cxx_status indentation
This revision was landed with ongoing or failed builds.May 4 2023, 11:07 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2023, 11:07 AM
chaitanyav added a subscriber: chaitanyav.EditedMay 4 2023, 11:14 AM

@shafik @aaron.ballman I see some errors

[2/2503] Building DiagnosticCrossTUKinds.inc...
FAILED: tools/clang/include/clang/Basic/DiagnosticCrossTUKinds.inc /home/nvellanki/scratch/llvm-project/tools/clang/include/clang/Basic/DiagnosticCrossTUKinds.inc 
cd /home/nvellanki/scratch/llvm-project && /home/nvellanki/scratch/llvm-project/bin/clang-tblgen -gen-clang-diags-defs -clang-component=CrossTU -I /home/nvellanki/scratch/llvm-project/clang/include/clang/Basic -I/home/nvellanki/scratch/llvm-project/clang/include -I/home/nvellanki/scratch/llvm-project/tools/clang/include -I/home/nvellanki/scratch/llvm-project/include -I/home/nvellanki/scratch/llvm-project/llvm/include /home/nvellanki/scratch/llvm-project/clang/include/clang/Basic/Diagnostic.td --write-if-changed -o tools/clang/include/clang/Basic/DiagnosticCrossTUKinds.inc -d tools/clang/include/clang/Basic/DiagnosticCrossTUKinds.inc.d
Included from /home/nvellanki/scratch/llvm-project/clang/include/clang/Basic/Diagnostic.td:168:
/home/nvellanki/scratch/llvm-project/clang/include/clang/Basic/DiagnosticSemaKinds.td:9443:11: error: Variable not defined: 'CXXPre2bCompat'
  InGroup<CXXPre2bCompat>, DefaultIgnore;
          ^
[3/2503] Building DiagnosticASTKinds.inc...
FAILED: tools/clang/include/clang/Basic/DiagnosticASTKinds.inc /home/nvellanki/scratch/llvm-project/tools/clang/include/clang/Basic/DiagnosticASTKinds.inc 
cd /home/nvellanki/scratch/llvm-project && /home/nvellanki/scratch/llvm-project/bin/clang-tblgen -gen-clang-diags-defs -clang-component=AST -I /home/nvellanki/scratch/llvm-project/clang/include/clang/Basic -I/home/nvellanki/scratch/llvm-project/clang/include -I/home/nvellanki/scratch/llvm-project/tools/clang/include -I/home/nvellanki/scratch/llvm-project/include -I/home/nvellanki/scratch/llvm-project/llvm/include /home/nvellanki/scratch/llvm-project/clang/include/clang/Basic/Diagnostic.td --write-if-changed -o tools/clang/include/clang/Basic/DiagnosticASTKinds.inc -d tools/clang/include/clang/Basic/DiagnosticASTKinds.inc.d
Included from /home/nvellanki/scratch/llvm-project/clang/include/clang/Basic/Diagnostic.td:168:
/home/nvellanki/scratch/llvm-project/clang/include/clang/Basic/DiagnosticSemaKinds.td:9443:11: error: Variable not defined: 'CXXPre2bCompat'
  InGroup<CXXPre2bCompat>, DefaultIgnore;
          ^
[4/2503] Building DiagnosticFrontendKinds.inc...
FAILED: tools/clang/include/clang/Basic/DiagnosticFrontendKinds.inc /home/nvellanki/scratch/llvm-project/tools/clang/include/clang/Basic/DiagnosticFrontendKinds.inc 
cd /home/nvellanki/scratch/llvm-project && /home/nvellanki/scratch/llvm-project/bin/clang-tblgen -gen-clang-diags-defs -clang-component=Frontend -I /home/nvellanki/scratch/llvm-project/clang/include/clang/Basic -I/home/nvellanki/scratch/llvm-project/clang/include -I/home/nvellanki/scratch/llvm-project/tools/clang/include -I/home/nvellanki/scratch/llvm-project/include -I/home/nvellanki/scratch/llvm-project/llvm/include /home/nvellanki/scratch/llvm-project/clang/include/clang/Basic/Diagnostic.td --write-if-changed -o tools/clang/include/clang/Basic/DiagnosticFrontendKinds.inc -d tools/clang/include/clang/Basic/DiagnosticFrontendKinds.inc.d
Included from /home/nvellanki/scratch/llvm-project/clang/include/clang/Basic/Diagnostic.td:168:
/home/nvellanki/scratch/llvm-project/clang/include/clang/Basic/DiagnosticSemaKinds.td:9443:11: error: Variable not defined: 'CXXPre2bCompat'
  InGroup<CXXPre2bCompat>, DefaultIgnore;
          ^
[5/2503] Building DiagnosticAnalysisKinds.inc...
FAILED: tools/clang/include/clang/Basic/DiagnosticAnalysisKinds.inc /home/nvellanki/scratch/llvm-project/tools/clang/include/clang/Basic/DiagnosticAnalysisKinds.inc 
cd /home/nvellanki/scratch/llvm-project && /home/nvellanki/scratch/llvm-project/bin/clang-tblgen -gen-clang-diags-defs -clang-component=Analysis -I /home/nvellanki/scratch/llvm-project/clang/include/clang/Basic -I/home/nvellanki/scratch/llvm-project/clang/include -I/home/nvellanki/scratch/llvm-project/tools/clang/include -I/home/nvellanki/scratch/llvm-project/include -I/home/nvellanki/scratch/llvm-project/llvm/include /home/nvellanki/scratch/llvm-project/clang/include/clang/Basic/Diagnostic.td --write-if-changed -o tools/clang/include/clang/Basic/DiagnosticAnalysisKinds.inc -d tools/clang/include/clang/Basic/DiagnosticAnalysisKinds.inc.d
Included from /home/nvellanki/scratch/llvm-project/clang/include/clang/Basic/Diagnostic.td:168:
/home/nvellanki/scratch/llvm-project/clang/include/clang/Basic/DiagnosticSemaKinds.td:9443:11: error: Variable not defined: 'CXXPre2bCompat'
  InGroup<CXXPre2bCompat>, DefaultIgnore;
          ^
[6/2503] Building DiagnosticDriverKinds.inc...
FAILED: tools/clang/include/clang/Basic/DiagnosticDriverKinds.inc /home/nvellanki/scratch/llvm-project/tools/clang/include/clang/Basic/DiagnosticDriverKinds.inc 
cd /home/nvellanki/scratch/llvm-project && /home/nvellanki/scratch/llvm-project/bin/clang-tblgen -gen-clang-diags-defs -clang-component=Driver -I /home/nvellanki/scratch/llvm-project/clang/include/clang/Basic -I/home/nvellanki/scratch/llvm-project/clang/include -I/home/nvellanki/scratch/llvm-project/tools/clang/include -I/home/nvellanki/scratch/llvm-project/include -I/home/nvellanki/scratch/llvm-project/llvm/include /home/nvellanki/scratch/llvm-project/clang/include/clang/Basic/Diagnostic.td --write-if-changed -o tools/clang/include/clang/Basic/DiagnosticDriverKinds.inc -d tools/clang/include/clang/Basic/DiagnosticDriverKinds.inc.d
Included from /home/nvellanki/scratch/llvm-project/clang/include/clang/Basic/Diagnostic.td:168:
/home/nvellanki/scratch/llvm-project/clang/include/clang/Basic/DiagnosticSemaKinds.td:9443:11: error: Variable not defined: 'CXXPre2bCompat'
  InGroup<CXXPre2bCompat>, DefaultIgnore;
          ^
[7/2503] Building DiagnosticCommentKinds.inc...
FAILED: tools/clang/include/clang/Basic/DiagnosticCommentKinds.inc /home/nvellanki/scratch/llvm-project/tools/clang/include/clang/Basic/DiagnosticCommentKinds.inc 
cd /home/nvellanki/scratch/llvm-project && /home/nvellanki/scratch/llvm-project/bin/clang-tblgen -gen-clang-diags-defs -clang-component=Comment -I /home/nvellanki/scratch/llvm-project/clang/include/clang/Basic -I/home/nvellanki/scratch/llvm-project/clang/include -I/home/nvellanki/scratch/llvm-project/tools/clang/include -I/home/nvellanki/scratch/llvm-project/include -I/home/nvellanki/scratch/llvm-project/llvm/include /home/nvellanki/scratch/llvm-project/clang/include/clang/Basic/Diagnostic.td --write-if-changed -o tools/clang/include/clang/Basic/DiagnosticCommentKinds.inc -d tools/clang/include/clang/Basic/DiagnosticCommentKinds.inc.d
Included from /home/nvellanki/scratch/llvm-project/clang/include/clang/Basic/Diagnostic.td:168:
/home/nvellanki/scratch/llvm-project/clang/include/clang/Basic/DiagnosticSemaKinds.td:9443:11: error: Variable not defined: 'CXXPre2bCompat'
  InGroup<CXXPre2bCompat>, DefaultIgnore;
          ^
[8/2503] Building DiagnosticCommonKinds.inc...
FAILED: tools/clang/include/clang/Basic/DiagnosticCommonKinds.inc /home/nvellanki/scratch/llvm-project/tools/clang/include/clang/Basic/DiagnosticCommonKinds.inc 
cd /home/nvellanki/scratch/llvm-project && /home/nvellanki/scratch/llvm-project/bin/clang-tblgen -gen-clang-diags-defs -clang-component=Common -I /home/nvellanki/scratch/llvm-project/clang/include/clang/Basic -I/home/nvellanki/scratch/llvm-project/clang/include -I/home/nvellanki/scratch/llvm-project/tools/clang/include -I/home/nvellanki/scratch/llvm-project/include -I/home/nvellanki/scratch/llvm-project/llvm/include /home/nvellanki/scratch/llvm-project/clang/include/clang/Basic/Diagnostic.td --write-if-changed -o tools/clang/include/clang/Basic/DiagnosticCommonKinds.inc -d tools/clang/include/clang/Basic/DiagnosticCommonKinds.inc.d
Included from /home/nvellanki/scratch/llvm-project/clang/include/clang/Basic/Diagnostic.td:168:
/home/nvellanki/scratch/llvm-project/clang/include/clang/Basic/DiagnosticSemaKinds.td:9443:11: error: Variable not defined: 'CXXPre2bCompat'
  InGroup<CXXPre2bCompat>, DefaultIgnore;
          ^
[9/2503] Building CXX object lib/LTO/CMakeFiles/LLVMLTO.dir/LTO.cpp.o
ninja: build stopped: subcommand failed.

This commit is failing couple of buildbots. Can you revert or fix?

I am about to fix it

This commit is failing couple of buildbots. Can you revert or fix?

Shafik is validating a fix for it right now, it should be fixed momentarily.

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

No worries, unfortunate timing on my part that our commits crossed.

h-vetinari added inline comments.
clang/www/cxx_status.html
1485–1486

That sentence is impossible to parse for me. Is "outside of defaulted special member functions" supposed to be a subclause? The second sentence is also not great ("Which include [X], allow [Y]").

Also typos: "memeber" and "paremeters".