Skip to content

[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

Merged
merged 4 commits into from
Jun 9, 2025
Merged

[Clang] Implement CWG2496 #142975

merged 4 commits into from
Jun 9, 2025

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
rorth pushed a commit to rorth/llvm-project that referenced this pull request Jun 11, 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() &;
};
```
DhruvSrivastavaX pushed a commit to DhruvSrivastavaX/lldb-for-aix that referenced this pull request Jun 12, 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() &;
};
```
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