[clang] Disable missing definition warning on pure virtual functions #74510

charmitro · 2023-12-05T18:31:31Z

Warning '-Wundefined-func-template' incorrectly indicates that no definition is available for a pure virtual function. However, a definition is not needed for a pure virtual function.

Fixes #74016

llvmbot · 2023-12-05T18:32:03Z

@llvm/pr-subscribers-clang

Author: Charalampos Mitrodimas (charmitro)

Changes

Warning '-Wundefined-func-template' incorrectly indicates that no definition is available for a pure virtual function. However, a definition is not needed for a pure virtual function.

Fixes #74016

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

2 Files Affected:

(modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+2-1)
(added) clang/test/SemaTemplate/instantiate-pure-virtual-function.cpp (+20)

diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index d768bb72e07c0..0850b2d64b632 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -4952,7 +4952,8 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
         std::make_pair(Function, PointOfInstantiation));
     } else if (TSK == TSK_ImplicitInstantiation) {
       if (AtEndOfTU && !getDiagnostics().hasErrorOccurred() &&
-          !getSourceManager().isInSystemHeader(PatternDecl->getBeginLoc())) {
+          !getSourceManager().isInSystemHeader(PatternDecl->getBeginLoc()) &&
+          !Function->isVirtualAsWritten()) {
         Diag(PointOfInstantiation, diag::warn_func_template_missing)
           << Function;
         Diag(PatternDecl->getLocation(), diag::note_forward_template_decl);
diff --git a/clang/test/SemaTemplate/instantiate-pure-virtual-function.cpp b/clang/test/SemaTemplate/instantiate-pure-virtual-function.cpp
new file mode 100644
index 0000000000000..7ae323343330e
--- /dev/null
+++ b/clang/test/SemaTemplate/instantiate-pure-virtual-function.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wundefined-func-template %s
+// expected-no-diagnostics
+
+namespace PR74016 {
+  template <typename T> class B {
+  public:
+    constexpr void foo(const T &) { bar(1); }
+    virtual constexpr void bar(unsigned int) = 0;
+  };
+
+  template <typename T> class D : public B<T> {
+  public:
+    constexpr void bar(unsigned int) override {}
+  };
+
+  void test() {
+    auto t = D<int>();
+    t.foo(0);
+  }
+};

Fznamznon

Perhaps needs a release note.

Fznamznon · 2023-12-05T18:38:53Z

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

@@ -4952,7 +4952,8 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
        std::make_pair(Function, PointOfInstantiation));
    } else if (TSK == TSK_ImplicitInstantiation) {
      if (AtEndOfTU && !getDiagnostics().hasErrorOccurred() &&
-          !getSourceManager().isInSystemHeader(PatternDecl->getBeginLoc())) {
+          !getSourceManager().isInSystemHeader(PatternDecl->getBeginLoc()) &&
+          !Function->isVirtualAsWritten()) {


Should we also check that it is pure? AFAIK isVirtualAsWritten returns true for function marked explicitly virtual. There is also FunctionDecl::isPure() to check that it is pure.

Fznamznon · 2023-12-05T18:39:57Z

clang/test/SemaTemplate/instantiate-pure-virtual-function.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -Wundefined-func-template %s
+// expected-no-diagnostics
+
+namespace PR74016 {


I think we normally call them GH when a bug was filed as a GitHub issue.

Suggested change

namespace PR74016 {

namespace GH74016 {

Fixed. I wasn't aware of this, thanks!

cor3ntin · 2023-12-05T20:14:17Z

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

@@ -4952,7 +4952,8 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
        std::make_pair(Function, PointOfInstantiation));
    } else if (TSK == TSK_ImplicitInstantiation) {
      if (AtEndOfTU && !getDiagnostics().hasErrorOccurred() &&
-          !getSourceManager().isInSystemHeader(PatternDecl->getBeginLoc())) {
+          !getSourceManager().isInSystemHeader(PatternDecl->getBeginLoc()) &&
+          !Function->isVirtualAsWritten() && !Function->isPure()) {


Suggested change

!Function->isVirtualAsWritten() && !Function->isPure()) {

!(Function->isVirtualAsWritten() && Function->isPure()) {

Can you add a test to make sure non-pure virtual function still get diagnosed?

After double-checking this, I think we should just check for !isPure() and don't bother checking for isVirtualAsWritten() at all since non-virtual functions cannot be pure. Also, for reference,

llvm-project/clang/include/clang/AST/Decl.h

Lines 2293 to 2295 in d6fbd96

/// Whether this virtual function is pure, i.e. makes the containing class

/// abstract.

bool isPure() const { return FunctionDeclBits.IsPure; }

Code updated to match this. Also a test-case added to verify that non-pure virtual functions still get diagnosed.

github-actions · 2023-12-05T21:50:03Z

✅ With the latest revision this PR passed the C/C++ code formatter.

cor3ntin · 2023-12-06T07:00:44Z

Can you add a release note indicating #74016 was fixed?
Otherwise LGTM.

If you wanted to make a follow up {R to rename isPure to isPureVirtual, i think that might be helpful (to avoid confusion with the gnu::pure attribute)

charmitro · 2023-12-06T08:37:07Z

Thank you both for your review!

Perhaps needs a release note.

Can you add a release note indicating #74016 was fixed? Otherwise LGTM.

Release note added.

If you wanted to make a follow up {R to rename isPure to isPureVirtual, i think that might be helpful (to avoid confusion with the gnu::pure attribute)

Yes, this could be helpful. I'll create a follow up PR for this.

cor3ntin

LGTM, thanks

ahatanak · 2024-01-05T02:03:53Z

Any progress on this patch?

As we discussed in #74016, we should make sure clang doesn't stop emitting the warning when the definition of a pure virtual function is actually needed.

It seems to me that we shouldn't set NeededForConstantEvaluation or NeedDefinition in Sema::MarkFunctionReferenced to true when a pure virtual function is used in a virtual function call.

AaronBallman

Aside from a minor NFC nit with where we decide to do the check, this LGTM

AaronBallman · 2024-01-11T16:04:54Z

clang/lib/Sema/SemaExpr.cpp

@@ -18931,7 +18931,7 @@ void Sema::MarkFunctionReferenced(SourceLocation Loc, FunctionDecl *Func,
  //   constant evaluated
  bool NeededForConstantEvaluation =
      isPotentiallyConstantEvaluatedContext(*this) &&
-      isImplicitlyDefinableConstexprFunction(Func);
+      isImplicitlyDefinableConstexprFunction(Func) && !Func->isPure();


I don't understand how the function being pure relates to whether it's needed for constant evaluation. I think this should move down to NeedDefinition below?

I concur, this does not make sense doing the check here.

Moving it to NeedDefinition silences the warnings of test call_pure_virtual_function_from_virtual, where we still require warnings for it, as mentioned in #74016 (comment).

I am not sure the diagnostic makes sense in that case since it can never actually be called.

Thank to @erichkeane I realized I am wrong, we can actually define a pure virtual function. So we do want to retain the diagnostic here.

I am not sure why moving it down silences the diagnostic, I would think doing (OdrUse == OdrUseContext::Used || NeededForConstantEvaluation || !Func->isPure()) should work.

Can you explain in more detail?

You're actually right.

I don't really recall what I was entering back when I was developing it, but it should work now.

github-actions · 2024-03-23T11:52:12Z

✅ With the latest revision this PR passed the Python code formatter.

Warning '-Wundefined-func-template' incorrectly indicates that no definition is available for a pure virtual function. However, a definition is not needed for a pure virtual function. Fixes llvm#74016 Signed-off-by: Charalampos Mitrodimas <[email protected]>

AaronBallman

LGTM! Do you need someone to land this on your behalf?

charmitro · 2024-04-09T13:39:45Z

LGTM! Do you need someone to land this on your behalf?

I don't have commit rights. If I remember correctly from when LLVM used Phabricator (reviews.llvm.org) you'd need my name & e-address. If that's so,
Charalampos Mitrodimas <[email protected]>

…lvm#74510) Warning '-Wundefined-func-template' incorrectly indicates that no definition is available for a pure virtual function. However, a definition is not needed for a pure virtual function. Fixes llvm#74016 (cherry picked from commit 0c92f86) Conflicts: clang/docs/ReleaseNotes.rst

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

Fznamznon reviewed Dec 5, 2023

View reviewed changes

charmitro force-pushed the pure-virtual-function-missing-def branch from 0ea96c4 to 0be0d65 Compare December 5, 2023 19:52

cor3ntin reviewed Dec 5, 2023

View reviewed changes

charmitro force-pushed the pure-virtual-function-missing-def branch from 0be0d65 to 1140e00 Compare December 5, 2023 21:47

charmitro force-pushed the pure-virtual-function-missing-def branch from 1140e00 to 5582cfa Compare December 5, 2023 21:53

charmitro force-pushed the pure-virtual-function-missing-def branch from 5582cfa to 83d29e8 Compare December 6, 2023 08:34

cor3ntin approved these changes Dec 6, 2023

View reviewed changes

charmitro force-pushed the pure-virtual-function-missing-def branch from e38ad9c to f60f12f Compare January 5, 2024 10:37

AaronBallman approved these changes Jan 11, 2024

View reviewed changes

charmitro force-pushed the pure-virtual-function-missing-def branch from f60f12f to 7961a90 Compare March 23, 2024 11:49

charmitro force-pushed the pure-virtual-function-missing-def branch from 7961a90 to 89eb978 Compare March 23, 2024 11:56

charmitro force-pushed the pure-virtual-function-missing-def branch 3 times, most recently from 958ba59 to c0ef5a0 Compare April 5, 2024 09:50

charmitro force-pushed the pure-virtual-function-missing-def branch from c0ef5a0 to 0348c13 Compare April 5, 2024 10:18

AaronBallman approved these changes Apr 9, 2024

View reviewed changes

AaronBallman merged commit 0c92f86 into llvm:main Apr 9, 2024

	!Function->isVirtualAsWritten() && !Function->isPure()) {
	!(Function->isVirtualAsWritten() && Function->isPure()) {

	/// Whether this virtual function is pure, i.e. makes the containing class
	/// abstract.
	bool isPure() const { return FunctionDeclBits.IsPure; }

[clang] Disable missing definition warning on pure virtual functions #74510

[clang] Disable missing definition warning on pure virtual functions #74510

Uh oh!

Conversation

charmitro commented Dec 5, 2023

Uh oh!

llvmbot commented Dec 5, 2023

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charmitro Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cor3ntin commented Dec 6, 2023

Uh oh!

charmitro commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

ahatanak commented Jan 5, 2024

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 23, 2024

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

charmitro commented Apr 9, 2024

Uh oh!

Uh oh!

charmitro Dec 5, 2023 •

edited

Loading

github-actions bot commented Dec 5, 2023 •

edited

Loading

charmitro commented Dec 6, 2023 •

edited

Loading