-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[Clang] Implement CWG2496 #142975
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Clang] Implement CWG2496 #142975
Conversation
https://cplusplus.github.io/CWG/issues/2496.html We failed to diagnose the following in C++23 mode ``` struct S { virtual void f(); // expected-note {{previous declaration is here}} }; struct T : S { virtual void f() &; }; ```
@llvm/pr-subscribers-clang Author: Corentin Jabot (cor3ntin) Changeshttps://cplusplus.github.io/CWG/issues/2496.html We failed to diagnose the following in C++23 mode
Full diff: https://github.com/llvm/llvm-project/pull/142975.diff 4 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 512071427b65c..d1ee6de66b4ee 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -155,6 +155,8 @@ Resolutions to C++ Defect Reports
- Implemented `CWG3005 Function parameters should never be name-independent <https://wg21.link/CWG3005>`_.
+- Implemented `CWG2496 ref-qualifiers and virtual overriding <https://wg21.link/CWG2496>`_.
+
C Language Changes
------------------
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 66f84fc67b52f..5a19dacdc4d84 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -1490,7 +1490,7 @@ static bool IsOverloadOrOverrideImpl(Sema &SemaRef, FunctionDecl *New,
// If the function is a class member, its signature includes the
// cv-qualifiers (if any) and ref-qualifier (if any) on the function itself.
auto DiagnoseInconsistentRefQualifiers = [&]() {
- if (SemaRef.LangOpts.CPlusPlus23)
+ if (SemaRef.LangOpts.CPlusPlus23 && !UseOverrideRules)
return false;
if (OldMethod->getRefQualifier() == NewMethod->getRefQualifier())
return false;
diff --git a/clang/test/CXX/drs/cwg24xx.cpp b/clang/test/CXX/drs/cwg24xx.cpp
index 9c9a3f14b9e8b..05b9a74e292ab 100644
--- a/clang/test/CXX/drs/cwg24xx.cpp
+++ b/clang/test/CXX/drs/cwg24xx.cpp
@@ -215,3 +215,23 @@ void (*q)() throw() = S();
// since-cxx17-error@-1 {{no viable conversion from 'S' to 'void (*)() throw()'}}
// since-cxx17-note@#cwg2486-conv {{candidate function}}
} // namespace cwg2486
+
+
+namespace cwg2496 { // cwg2496: 21
+#if __cplusplus >= 201102L
+struct S {
+ virtual void f(); // expected-note {{previous declaration is here}}
+ virtual void g() &; // expected-note {{previous declaration is here}}
+ virtual void h(); // expected-note {{previous declaration is here}}
+};
+
+struct T : S {
+ virtual void f() &;
+ // expected-error@-1 {{cannot overload a member function with ref-qualifier '&' with a member function without a ref-qualifier}}
+ virtual void g();
+ // expected-error@-1 {{cannot overload a member function without a ref-qualifier with a member function with ref-qualifier '&'}}
+ virtual void h() &&;
+ // expected-error@-1 {{cannot overload a member function with ref-qualifier '&&' with a member function without a ref-qualifier}}
+};
+#endif
+}
diff --git a/clang/www/cxx_dr_status.html b/clang/www/cxx_dr_status.html
index 58286db165077..fe9b571b47261 100755
--- a/clang/www/cxx_dr_status.html
+++ b/clang/www/cxx_dr_status.html
@@ -14811,7 +14811,7 @@ <h2 id="cxxdr">C++ defect report implementation status</h2>
<td><a href="https://cplusplus.github.io/CWG/issues/2496.html">2496</a></td>
<td>CD6</td>
<td><I>ref-qualifier</I>s and virtual overriding</td>
- <td class="unknown" align="center">Unknown</td>
+ <td class="unreleased" align="center">Clang 21</td>
</tr>
<tr class="open" id="2497">
<td><a href="https://cplusplus.github.io/CWG/issues/2497.html">2497</a></td>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you test full 3x3 matrix of combinations of ref-qualifiers between overriding and overridden functions?
clang/test/CXX/drs/cwg24xx.cpp
Outdated
namespace cwg2496 { // cwg2496: 21 | ||
#if __cplusplus >= 201102L | ||
struct S { | ||
virtual void f(); // expected-note {{previous declaration is here}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leave a bookmark here, and move the notes after their respective errors
// expected-note@#cwg2496-h {{previous declaration is here}} | ||
virtual void i(); | ||
virtual void j() &; | ||
virtual void k() &; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be diagnosed? k() &&
is in the base class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the two functions don't correspond.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see that wording now, you're right. And since they don't correspond, those are overloads not overrides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you leave a comment in the test about that?
virtual void f() &; | ||
// expected-error@-1 {{cannot overload a member function with ref-qualifier '&' with a member function without a ref-qualifier}} | ||
// expected-note@#cwg2496-f {{previous declaration is here}} | ||
virtual void g(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also like to see a case where it's declared as &
in the base class and &&
in the derived class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is l()
case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i added the l case afterwards :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
https://cplusplus.github.io/CWG/issues/2496.html We failed to diagnose the following in C++23 mode ``` struct S { virtual void f(); // expected-note {{previous declaration is here}} }; struct T : S { virtual void f() &; }; ```
https://cplusplus.github.io/CWG/issues/2496.html We failed to diagnose the following in C++23 mode ``` struct S { virtual void f(); // expected-note {{previous declaration is here}} }; struct T : S { virtual void f() &; }; ```
https://cplusplus.github.io/CWG/issues/2496.html We failed to diagnose the following in C++23 mode ``` struct S { virtual void f(); // expected-note {{previous declaration is here}} }; struct T : S { virtual void f() &; }; ```
https://cplusplus.github.io/CWG/issues/2496.html
We failed to diagnose the following in C++23 mode