Skip to content

Conversation

cor3ntin
Copy link
Contributor

@cor3ntin cor3ntin commented Jun 5, 2025

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() &;
};
```
@cor3ntin cor3ntin requested a review from erichkeane June 5, 2025 14:20
@cor3ntin cor3ntin requested a review from Endilll as a code owner June 5, 2025 14:20
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 5, 2025
@llvmbot
Copy link
Member

llvmbot commented Jun 5, 2025

@llvm/pr-subscribers-clang

Author: Corentin Jabot (cor3ntin)

Changes

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() &;
};

Full diff: https://github.com/llvm/llvm-project/pull/142975.diff

4 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+2)
  • (modified) clang/lib/Sema/SemaOverload.cpp (+1-1)
  • (modified) clang/test/CXX/drs/cwg24xx.cpp (+20)
  • (modified) clang/www/cxx_dr_status.html (+1-1)
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>

Copy link
Contributor

@Endilll Endilll left a 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?

namespace cwg2496 { // cwg2496: 21
#if __cplusplus >= 201102L
struct S {
virtual void f(); // expected-note {{previous declaration is here}}
Copy link
Contributor

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

@cor3ntin cor3ntin requested a review from AaronBallman June 9, 2025 15:14
// expected-note@#cwg2496-h {{previous declaration is here}}
virtual void i();
virtual void j() &;
virtual void k() &;
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

Copy link
Contributor

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();
Copy link
Collaborator

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.

Copy link
Contributor

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

Copy link
Contributor Author

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 :)

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@cor3ntin cor3ntin merged commit c800979 into llvm:main Jun 9, 2025
8 checks passed
tomtor pushed a commit to tomtor/llvm-project that referenced this pull request Jun 14, 2025
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() &;
};
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants