[Clang][Sema] Fix err_constexpr_virtual_base diagnostic so that it only diagnoses on constructors and destructors #163690
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ly diagnoses on constructors and destructors When this diagnostic was implemented it fired for all non-static data members. I added checking so that it only fires for contructors and destructors before C++26 (which now allows this). Some special care was required for the destructor case since constexpr constuctors were not allowed in C++11. Modified existing test to cover the new conditions. Fixes: llvm#97266
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Shafik Yaghmour (shafik) ChangesWhen this diagnostic was implemented it fired for all non-static data members. I added checking so that it only fires for constructors and destructors before C++26 (which now allows this). Some special care was required for the destructor case since constexpr constuctors were not allowed in C++11. Modified existing test to cover the new conditions. Also filed bug report on bad diagnostic found while writing the tests: #163683 Fixes: #97266 Full diff: https://github.com/llvm/llvm-project/pull/163690.diff 3 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 40bc7b9a4e45e..0eae2b76ff9b9 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3026,7 +3026,7 @@ def warn_cxx17_compat_constexpr_virtual : Warning<
"virtual constexpr functions are incompatible with "
"C++ standards before C++20">, InGroup<CXXPre20Compat>, DefaultIgnore;
def err_constexpr_virtual_base : Error<
- "constexpr %select{member function|constructor}0 not allowed in "
+ "constexpr %select{destructor|constructor}0 not allowed in "
"%select{struct|interface|class}1 with virtual base "
"%plural{1:class|:classes}2">;
def note_non_literal_incomplete : Note<
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index d41ab126c426f..da94b0070dfcb 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -1918,8 +1918,10 @@ static bool CheckConstexprMissingReturn(Sema &SemaRef, const FunctionDecl *Dcl);
bool Sema::CheckConstexprFunctionDefinition(const FunctionDecl *NewFD,
CheckConstexprKind Kind) {
- const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(NewFD);
- if (MD && MD->isInstance()) {
+ if ((!getLangOpts().CPlusPlus26 && isa<CXXConstructorDecl>(NewFD)) ||
+ ((getLangOpts().CPlusPlus20 && !getLangOpts().CPlusPlus26) &&
+ isa<CXXDestructorDecl>(NewFD))) {
+ const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(NewFD);
// C++11 [dcl.constexpr]p4:
// The definition of a constexpr constructor shall satisfy the following
// constraints:
@@ -1933,8 +1935,8 @@ bool Sema::CheckConstexprFunctionDefinition(const FunctionDecl *NewFD,
return false;
Diag(NewFD->getLocation(), diag::err_constexpr_virtual_base)
- << isa<CXXConstructorDecl>(NewFD)
- << getRecordDiagFromTagKind(RD->getTagKind()) << RD->getNumVBases();
+ << isa<CXXConstructorDecl>(NewFD)
+ << getRecordDiagFromTagKind(RD->getTagKind()) << RD->getNumVBases();
for (const auto &I : RD->vbases())
Diag(I.getBeginLoc(), diag::note_constexpr_virtual_base_here)
<< I.getSourceRange();
diff --git a/clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp b/clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp
index 51990ee4341d2..5a06d686b4c0f 100644
--- a/clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp
+++ b/clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp
@@ -84,10 +84,16 @@ struct T3 {
struct U {
constexpr U SelfReturn() const;
constexpr int SelfParam(U) const;
+ constexpr ~U(); // beforecxx20-error {{destructor cannot be declared constexpr}}
};
-struct V : virtual U { // expected-note {{here}}
- constexpr int F() const { return 0; } // expected-error {{constexpr member function not allowed in struct with virtual base class}}
+struct V : virtual U { // expected-note {{here}} //aftercxx20-note {{here}}
+ constexpr V() {} // expected-error {{constexpr constructor not allowed in struct with virtual base class}}
+ constexpr ~V() {} // aftercxx20-error {{constexpr destructor not allowed in struct with virtual base class}}
+ // beforecxx20-error@-1 {{destructor cannot be declared constexpr}}
+ // beforecxx14-error@-2 {{constexpr function's return type 'void' is not a literal type}}
+ // ^ FIXME this is just a bad diagnostic
+ constexpr int f() const { return 0; }
};
// or a compound-statememt that contains only [CXX11]
|
cor3ntin
reviewed
Oct 16, 2025
clang/lib/Sema/SemaDeclCXX.cpp
Outdated
Comment on lines
1921
to
1924
| if ((!getLangOpts().CPlusPlus26 && isa<CXXConstructorDecl>(NewFD)) || | ||
| ((getLangOpts().CPlusPlus20 && !getLangOpts().CPlusPlus26) && | ||
| isa<CXXDestructorDecl>(NewFD))) { | ||
| const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(NewFD); |
There was a problem hiding this comment.
This feels like you are trying to partially implement https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2025/p3533r2.html as a drive-by?
There was a problem hiding this comment.
That is not really my intent but leaving it enabled for C++26 is also not correct. I can leave it enabled for C++26 but it is such a trivial addition it seems silly not to. If the objections are strong I can take out the C++26 check.
Fznamznon
reviewed
Oct 16, 2025
| const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(NewFD); | ||
| if (MD && MD->isInstance()) { | ||
| if ((!getLangOpts().CPlusPlus26 && isa<CXXConstructorDecl>(NewFD)) || | ||
| ((getLangOpts().CPlusPlus20 && !getLangOpts().CPlusPlus26) && |
There was a problem hiding this comment.
Does it make sense to check for C++20? I think some other function will diagnose that destructor can't be constexpr.
There was a problem hiding this comment.
We don't have explicit constexpr destructor before C++20 and so this would be a spurious diagnostic.
Fznamznon
approved these changes
Oct 17, 2025
|
if we are doing something special for C++26, I'd like to see tests for it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When this diagnostic was implemented it fired for all non-static data members. I added checking so that it only fires for constructors and destructors before C++26 (which now allows this). Some special care was required for the destructor case since constexpr constuctors were not allowed in C++11. Modified existing test to cover the new conditions.
Also filed bug report on bad diagnostic found while writing the tests: #163683
Fixes: #97266