Skip to content

Conversation

@Lancern
Copy link
Member

@Lancern Lancern commented Dec 22, 2023

According to CWG1351, overriding functions that are defined as deleted can have more lax exception specifications compared to the base version. For example, the following code should compile:

struct B { virtual void h() noexcept = delete; }; struct D: B { void h() override = delete; // Should be OK };

Clang incorrectly reports that the exception specification of D::h is more lax than base version. This patch fixes this.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Dec 22, 2023
@llvmbot
Copy link
Member

llvmbot commented Dec 22, 2023

@llvm/pr-subscribers-clang

Author: Sirui Mu (Lancern)

Changes

According to CWG1351, overriding functions that are defined as deleted can have more lax exception specifications compared to the base version. For example, the following code should compile:

struct B { virtual void h() noexcept = delete; }; struct D: B { void h() override = delete; // Should be OK };

Clang incorrectly reports that the exception specification of D::h is more lax than base version. This patch fixes this.


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

2 Files Affected:

  • (modified) clang/lib/Sema/SemaExceptionSpec.cpp (+5)
  • (modified) clang/test/CXX/except/except.spec/p5-virtual.cpp (+4)
diff --git a/clang/lib/Sema/SemaExceptionSpec.cpp b/clang/lib/Sema/SemaExceptionSpec.cpp index 75730ea888afb4..f8f9c9d491a18e 100644 --- a/clang/lib/Sema/SemaExceptionSpec.cpp +++ b/clang/lib/Sema/SemaExceptionSpec.cpp @@ -979,6 +979,11 @@ bool Sema::CheckOverridingFunctionExceptionSpec(const CXXMethodDecl *New, if (isa<CXXDestructorDecl>(New) && New->getParent()->isDependentType()) return false; + // CWG1351: if either of the old function or the new function is defined as + // deleted, we don't need this check. + if (Old->isDeleted() || New->isDeleted()) + return false; + // If the old exception specification hasn't been parsed yet, or the new // exception specification can't be computed yet, remember that we need to // perform this check when we get to the end of the outermost diff --git a/clang/test/CXX/except/except.spec/p5-virtual.cpp b/clang/test/CXX/except/except.spec/p5-virtual.cpp index 69daec6ee53347..2cecd6ce51f698 100644 --- a/clang/test/CXX/except/except.spec/p5-virtual.cpp +++ b/clang/test/CXX/except/except.spec/p5-virtual.cpp @@ -45,6 +45,8 @@ struct Base virtual void f15(); virtual void f16(); + virtual void f17() noexcept = delete; + virtual void g1() throw(); // expected-note {{overridden virtual function is here}} virtual void g2() throw(int); // expected-note {{overridden virtual function is here}} virtual void g3() throw(A); // expected-note {{overridden virtual function is here}} @@ -81,6 +83,8 @@ struct Derived : Base virtual void f15() noexcept; virtual void f16() throw(); + virtual void f17() = delete; + virtual void g1() throw(int); // expected-error {{exception specification of overriding function is more lax}} virtual void g2(); // expected-error {{exception specification of overriding function is more lax}} virtual void g3() throw(D); // expected-error {{exception specification of overriding function is more lax}} 

// CWG1351: if either of the old function or the new function is defined as
// deleted, we don't need this check.
if (Old->isDeleted() || New->isDeleted())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Either they both have to be deleted or not. We should diagnose otherwise. So I think checking both is redundant.

@Lancern
Copy link
Member Author

Lancern commented Dec 29, 2023

Oops, code formatter formats unrelated code. Revert and re-commit.

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

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

This needs a release note. Also, since this is implementing a core issue, this needs to update a DRs test as well.

@Lancern Lancern requested a review from Endilll as a code owner January 4, 2024 14:48
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.

You need to run clang/www/make_cxx_drs script in order to make cxx_dr_status.html reflect that CWG1351 is now implemented.

#endif
}

namespace dr1351 { // dr1351: 1
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you intended // dr1351: 18.

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

5 participants