Skip to content

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

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
473e8bb
[clang][Sema] Refine unused-member-function diagnostic message for co…
guillem-bartrina-sonarsource Mar 8, 2024
0a73493
Remove constructor name from diagnostic
guillem-bartrina-sonarsource Apr 5, 2024
0e5e098
Add release note
guillem-bartrina-sonarsource Apr 5, 2024
e53a966
Rephrase release note
guillem-bartrina-sonarsource Jun 19, 2024
edf945a
Merge branch 'main' into gb/unused-constructor
guillem-bartrina-sonarsource Jun 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,8 @@ Improvements to Clang's diagnostics
- Clang no longer emits a "declared here" note for a builtin function that has no declaration in source.
Fixes #GH93369.

- Clang now diagnoses ``-Wunused-member-function`` with a specific message for constructors.

Improvements to Clang's time-trace
----------------------------------

Expand Down
2 changes: 1 addition & 1 deletion clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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">,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

@guillem-bartrina-sonarsource guillem-bartrina-sonarsource May 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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;

Copy link
Author

Choose a reason for hiding this comment

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

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”.

InGroup<UnusedMemberFunction>, DefaultIgnore;
def warn_used_but_marked_unused: Warning<"%0 was marked unused but was used">,
InGroup<UsedButMarkedUnused>, DefaultIgnore;
Expand Down
16 changes: 11 additions & 5 deletions clang/lib/Sema/Sema.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1443,11 +1443,17 @@ 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<CXXConstructorDecl>(DiagD))
Diag(DiagD->getLocation(), diag::warn_unused_member_function)
<< /*constructor=*/0 << DiagRange;
else if (isa<CXXMethodDecl>(DiagD))
Diag(DiagD->getLocation(), diag::warn_unused_member_function)
<< /*member function=*/1 << DiagD << DiagRange;
else
Diag(DiagD->getLocation(), diag::warn_unused_function)
<< DiagD << DiagRange;
}
}
} else {
const VarDecl *DiagD = cast<VarDecl>(*I)->getDefinition();
Expand Down
23 changes: 23 additions & 0 deletions clang/test/SemaCXX/warn-unused-filescoped.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,33 @@ struct S {
struct SVS : public VS {
void vm() { }
};

struct CS {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

CS() {}
CS(bool a) {}
CS(int b) {} // expected-warning{{unused constructor}}
CS(float c);
};

struct DCS : public CS {
DCS() = default; // expected-warning{{unused constructor}}
DCS(bool a) : CS(a) {} // expected-warning{{unused constructor}}
DCS(const DCS&) {}
DCS(DCS&&) {} // expected-warning{{unused constructor}}
};

template<typename T>
struct TCS {
TCS();
};
template <typename T> TCS<T>::TCS() {}
template <> TCS<int>::TCS() {} // expected-warning{{unused constructor}}
}

void S::m3() {} // expected-warning{{unused member function 'm3'}}

CS::CS(float c) {} // expected-warning{{unused constructor}}

static inline void f4() {} // expected-warning{{unused function 'f4'}}
const unsigned int cx = 0; // expected-warning{{unused variable 'cx'}}
const unsigned int cy = 0;
Expand Down
Loading