[clang][Sema] Refine unused-member-function diagnostic message for constructors #84515

guillem-bartrina-sonarsource · 2024-03-08T16:26:25Z

The current diagnostic message is unused member function A for every kind of method, including constructors. The idea is to refine the message to unused constructor when the method is a constructor.

github-actions · 2024-03-08T16:26:42Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-03-08T16:27:07Z

@llvm/pr-subscribers-clang

Author: None (guillem-bartina-sonarsource)

Changes

The current diagnostic message is unused member function A for every kind of method, including constructors. The idea is to refine the message to unused constructor A when the method is a constructor.

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

3 Files Affected:

(modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+1-1)
(modified) clang/lib/Sema/Sema.cpp (+10-5)
(modified) clang/test/SemaCXX/warn-unused-filescoped.cpp (+23)

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 6da49facd27ec2..770bda6b3ba193 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -400,7 +400,7 @@ def warn_unused_function : Warning<"unused function %0">,
   InGroup<UnusedFunction>, DefaultIgnore;
 def warn_unused_template : Warning<"unused %select{function|variable}0 template %1">,
   InGroup<UnusedTemplate>, DefaultIgnore;
-def warn_unused_member_function : Warning<"unused member function %0">,
+def warn_unused_member_function : Warning<"unused %select{member function|constructor}0 %1">,
   InGroup<UnusedMemberFunction>, DefaultIgnore;
 def warn_used_but_marked_unused: Warning<"%0 was marked unused but was used">,
   InGroup<UsedButMarkedUnused>, DefaultIgnore;
diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
index 720d5fd5f0428d..163ab48998d5e0 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -1398,11 +1398,16 @@ void Sema::ActOnEndOfTranslationUnit() {
           if (FD->getDescribedFunctionTemplate())
             Diag(DiagD->getLocation(), diag::warn_unused_template)
                 << /*function=*/0 << DiagD << DiagRange;
-          else
-            Diag(DiagD->getLocation(), isa<CXXMethodDecl>(DiagD)
-                                           ? diag::warn_unused_member_function
-                                           : diag::warn_unused_function)
-                << DiagD << DiagRange;
+          else {
+            if (isa<CXXMethodDecl>(DiagD))
+              Diag(DiagD->getLocation(), diag::warn_unused_member_function)
+                  << (!isa<CXXConstructorDecl>(DiagD) ? /*member function=*/0
+                                                      : /*constructor=*/1)
+                  << DiagD << DiagRange;
+            else
+              Diag(DiagD->getLocation(), diag::warn_unused_function)
+                  << DiagD << DiagRange;
+          }
         }
       } else {
         const VarDecl *DiagD = cast<VarDecl>(*I)->getDefinition();
diff --git a/clang/test/SemaCXX/warn-unused-filescoped.cpp b/clang/test/SemaCXX/warn-unused-filescoped.cpp
index be8d350855c078..b3d1bb4661a5f4 100644
--- a/clang/test/SemaCXX/warn-unused-filescoped.cpp
+++ b/clang/test/SemaCXX/warn-unused-filescoped.cpp
@@ -76,10 +76,33 @@ struct S {
   struct SVS : public VS {
     void vm() { }
   };
+
+  struct CS {
+    CS() {}
+    CS(bool a) {}
+    CS(int b) {} // expected-warning{{unused constructor 'CS'}}
+    CS(float c);
+  };
+
+  struct DCS : public CS {
+    DCS() = default; // expected-warning{{unused constructor 'DCS'}}
+    DCS(bool a) : CS(a) {} // expected-warning{{unused constructor 'DCS'}}
+    DCS(const DCS&) {}
+    DCS(DCS&&) {} // expected-warning{{unused constructor 'DCS'}}
+  };
+
+  template<typename T>
+  struct TCS {
+    TCS();
+  };
+  template <typename T> TCS<T>::TCS() {}
+  template <> TCS<int>::TCS() {} // expected-warning{{unused constructor 'TCS'}}
 }
 
 void S::m3() {} // expected-warning{{unused member function 'm3'}}
 
+CS::CS(float c) {} // expected-warning{{unused constructor 'CS'}}
+
 static inline void f4() {} // expected-warning{{unused function 'f4'}}
 const unsigned int cx = 0; // expected-warning{{unused variable 'cx'}}
 const unsigned int cy = 0;

shafik · 2024-03-09T01:12:36Z

Looks like the build failed b/c you did not run git clang-format

cor3ntin · 2024-03-09T11:47:13Z

clang/lib/Sema/Sema.cpp

+          else {
+            if (isa<CXXMethodDecl>(DiagD))
+              Diag(DiagD->getLocation(), diag::warn_unused_member_function)
+                  << (!isa<CXXConstructorDecl>(DiagD) ? /*member function=*/0
+                                                      : /*constructor=*/1)
+                  << DiagD << DiagRange;
+            else
+              Diag(DiagD->getLocation(), diag::warn_unused_function)
+                  << DiagD << DiagRange;
+          }


Suggested change

else {

if (isa<CXXMethodDecl>(DiagD))

Diag(DiagD->getLocation(), diag::warn_unused_member_function)

<< (!isa<CXXConstructorDecl>(DiagD) ? /*member function=*/0

: /*constructor=*/1)

<< DiagD << DiagRange;

else

Diag(DiagD->getLocation(), diag::warn_unused_function)

<< DiagD << DiagRange;

}

else {

Diag(DiagD->getLocation(), isa<CXXMethodDecl>(DiagD)

? diag::warn_unused_member_function

: diag::warn_unused_function)

<< DiagD

<< (!isa<CXXConstructorDecl>(DiagD) ? /*member function=*/0

: /*constructor=*/1

<< DiagRange;

}

I think you can simplify like that.
And then change warn_unused_member_function : Warning<"unused %select{member function|constructor}1 %0"

Note that warn_unused_function and warn_unused_member_function are two different diagnostics, with different messages. The former has only one argument, whereas the latter now has two (one of them selects between member function and constructor).
Unless I'm missing some hidden logic regarding diagnostic builders and the '<<' operator, we can't merge the two because the two diagnostics have different number of arguments.

…nstructors

guillem-bartrina-sonarsource · 2024-03-19T10:32:37Z

Ping

clang/test/SemaCXX/warn-unused-filescoped.cpp

hazohelet · 2024-03-21T05:48:24Z

Please add a release note line to clang/docs/ReleaseNotes.rst because this patch changes the diagnostic behavior of clang

guillem-bartrina-sonarsource · 2024-06-20T07:17:19Z

Ping

steakhal · 2024-07-01T14:02:07Z

Ping.

guillem-bartrina-sonarsource · 2024-07-25T12:36:28Z

Ping.

AaronBallman

Thank you for the diagnostic improvement!

AaronBallman · 2024-07-25T12:59:42Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -402,7 +402,7 @@ def warn_unused_function : Warning<"unused function %0">,
  InGroup<UnusedFunction>, DefaultIgnore;
 def warn_unused_template : Warning<"unused %select{function|variable}0 template %1">,
  InGroup<UnusedTemplate>, DefaultIgnore;
-def warn_unused_member_function : Warning<"unused member function %0">,
+def warn_unused_member_function : Warning<"unused %select{constructor|member function %1}0">,


I would recommend using %sub{select_special_member_kind}1 instead of manually spelling out constructor; see note_member_synthesized_at for an example of it being used.

Hello @AaronBallman, and thanks for this recommendation.

I clearly see the advantages it brings, namely refining the message also for other special members and avoiding having to do the type test for the different nodes at the diagnostic creation site.

However, note that only default constructors are considered “special members” (see CXXSpecialMemberKind). Therefore, using this trick would not avoid having to manually spell constructor in the template because it would still be necessary to treat them specially. The trick would just refine the message for other special members with little additional effort.

The template would then be something like

def warn_unused_member_function : Warning<"unused %select{" "member function %1|" "constructor|" "%sub{select_special_member_kind}1}0">, InGroup<UnusedMemberFunction>, DefaultIgnore;

And the implementation

DefaultedFunctionKind DFK = getDefaultedFunctionKind(DiagD); if (DFK.isSpecialMember()) Diag(DiagD->getLocation(), diag::warn_unused_member_function) << /*select special member kind=*/2 << llvm::to_underlying(DFK.asSpecialMember()) << DiagRange; else if (isa<CXXConstructorDecl>(DiagD)) Diag(DiagD->getLocation(), diag::warn_unused_member_function) << /*constructor=*/1 << DiagRange; else if (isa<CXXMethodDecl>(DiagD)) Diag(DiagD->getLocation(), diag::warn_unused_member_function) << /*member function=*/0 << DiagD << DiagRange; else Diag(DiagD->getLocation(), diag::warn_unused_function) << DiagD << DiagRange;

WDYT?

However, note that only default constructors are considered “special members” (see CXXSpecialMemberKind).

It supports more than just a default constructor:

llvm-project/clang/include/clang/Sema/Sema.h

Line 422 in d0fb835

enum class CXXSpecialMemberKind {

So I was thinking something more like:

def warn_unused_member_function : Warning< "unused %select{member function %1|%sub{select_special_member_kind}1}0">, InGroup<UnusedMemberFunction>, DefaultIgnore;

and

DefaultedFunctionKind DFK = getDefaultedFunctionKind(DiagD); if (DFK.isSpecialMember()) Diag(DiagD->getLocation(), diag::warn_unused_member_function) << /*special member*/1 << DFK.asSpecialMember()) << DiagRange; else if (isa<CXXMethodDecl>(DiagD)) Diag(DiagD->getLocation(), diag::warn_unused_member_function) << /*member*/0 << DiagD << DiagRange; else Diag(DiagD->getLocation(), diag::warn_unused_function) << DiagD << DiagRange;

Yes, select_special_member_kind supports more than the default constructor: all other special members (copy constructor, move constructor, copy assignment operator, move assignment operator and destructor).

However, this PR was originally to refine the diagnostic message for all constructors, not just the default, copy and move ones.

With your approach, which only uses select_special_member_kind,

struct S { S() = default; // <--- S(int) {} S(float); S(const S &) {} // <--- S(S &&) {} // <--- }

Only the message will be refined for constructors with an arrow, because the other constructors are not considered “special members”.

AaronBallman · 2024-07-25T13:00:57Z

clang/test/SemaCXX/warn-unused-filescoped.cpp

@@ -76,10 +76,33 @@ struct S {
  struct SVS : public VS {
    void vm() { }
  };
+
+  struct CS {


When switching the diagnostic approach, be sure to add test coverage for unused copy/move constructors, copy/move assignment, and destructors (you can get an unused destructor if you only use new to allocate the class rather than stack allocate it).

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Mar 8, 2024

shafik requested review from hazohelet, erichkeane and Endilll March 9, 2024 01:12

cor3ntin reviewed Mar 9, 2024

View reviewed changes

[clang][Sema] Refine unused-member-function diagnostic message for co…

473e8bb

…nstructors

guillem-bartrina-sonarsource force-pushed the gb/unused-constructor branch from f8f93fe to 473e8bb Compare March 10, 2024 12:59

tbaederr reviewed Mar 19, 2024

View reviewed changes

clang/test/SemaCXX/warn-unused-filescoped.cpp Outdated Show resolved Hide resolved

Endilll removed their request for review March 23, 2024 21:38

guillem-bartrina-sonarsource added 4 commits April 5, 2024 18:19

Remove constructor name from diagnostic

0a73493

Add release note

0e5e098

Rephrase release note

e53a966

Merge branch 'main' into gb/unused-constructor

edf945a

steakhal requested a review from AaronBallman July 1, 2024 14:02

AaronBallman reviewed Jul 25, 2024

View reviewed changes

[clang][Sema] Refine unused-member-function diagnostic message for constructors #84515

Are you sure you want to change the base?

[clang][Sema] Refine unused-member-function diagnostic message for constructors #84515

Conversation

guillem-bartrina-sonarsource commented Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 8, 2024

Uh oh!

llvmbot commented Mar 8, 2024

Uh oh!

shafik commented Mar 9, 2024

Uh oh!

cor3ntin Mar 9, 2024

Choose a reason for hiding this comment

Uh oh!

guillem-bartrina-sonarsource Mar 10, 2024 • 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!

guillem-bartrina-sonarsource commented Mar 19, 2024

Uh oh!

Uh oh!

hazohelet commented Mar 21, 2024

Uh oh!

guillem-bartrina-sonarsource commented Jun 20, 2024

Uh oh!

steakhal commented Jul 1, 2024

Uh oh!

guillem-bartrina-sonarsource commented Jul 25, 2024

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

AaronBallman Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

guillem-bartrina-sonarsource May 29, 2025 • 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!

AaronBallman Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

guillem-bartrina-sonarsource Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

AaronBallman Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

guillem-bartrina-sonarsource commented Mar 8, 2024 •

edited

Loading

guillem-bartrina-sonarsource Mar 10, 2024 •

edited

Loading

guillem-bartrina-sonarsource May 29, 2025 •

edited

Loading