Hi,
Please find in attach a second version of the patch, which reuses the cache
of defined records properly.
With this patch, -Wunused-member-function now flags unused private methods
whenever the class is fully defined.
This patch does not attempt to fix false positives that were being triggered
before in classes in anonymous namespaces. I'll fix those afterwards.
OK to commit?
Thanks,
Nuno
----- Original Message -----
From: Nuno Lopes
Sent: Sunday, March 23, 2014 11:14 PM
Subject: Improving -Wunused-member-function
Hi,
I would like to improve -Wunused-member-function to detect unused private
methods, similarly to how -Wunused-private-fields works.
I think clang should be able to flag a method if 1) it is unused, 2) all
the
methods of the class are defined in the TU, and 3) any of the following
conditions holds:
- The method is private.
- The method is protected and the class is final.
- The method is public and the class is in an anonymous namespace.
I have a simple implementation in attach that can handle the first case
(private methods) only.
I'm not very happy with it, though. In particular I would like to move the
logic somewhere else, so that we can reuse it from Codegen. And right now
I'm not caching things properly. Any suggestions to where this code
belongs? Should it go directly to Decl? (but that would imply adding a
few
fields for cache purposes).
Any comments and suggestions are welcomed!
Thanks,
Nuno
P.S.: I run the attached patch over the LLVM codebase and I already fixed
a
bunch of cases it detected (but left many still). So big code bases will
certainly benefit from this analysis. Moreover, removing unused decls
triggered more -Wunused-private-fields warnings.
Index: lib/Sema/Sema.cpp
===================================================================
--- lib/Sema/Sema.cpp (revision 206567)
+++ lib/Sema/Sema.cpp (working copy)
@@ -362,54 +362,7 @@
return CK_Invalid;
}
-/// \brief Used to prune the decls of Sema's UnusedFileScopedDecls vector.
-static bool ShouldRemoveFromUnused(Sema *SemaRef, const DeclaratorDecl *D) {
- if (D->getMostRecentDecl()->isUsed())
- return true;
- if (D->isExternallyVisible())
- return true;
-
- if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
- // UnusedFileScopedDecls stores the first declaration.
- // The declaration may have become definition so check again.
- const FunctionDecl *DeclToCheck;
- if (FD->hasBody(DeclToCheck))
- return !SemaRef->ShouldWarnIfUnusedFileScopedDecl(DeclToCheck);
-
- // Later redecls may add new information resulting in not having to warn,
- // so check again.
- DeclToCheck = FD->getMostRecentDecl();
- if (DeclToCheck != FD)
- return !SemaRef->ShouldWarnIfUnusedFileScopedDecl(DeclToCheck);
- }
-
- if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {
- // If a variable usable in constant expressions is referenced,
- // don't warn if it isn't used: if the value of a variable is required
- // for the computation of a constant expression, it doesn't make sense to
- // warn even if the variable isn't odr-used. (isReferenced doesn't
- // precisely reflect that, but it's a decent approximation.)
- if (VD->isReferenced() &&
- VD->isUsableInConstantExpressions(SemaRef->Context))
- return true;
-
- // UnusedFileScopedDecls stores the first declaration.
- // The declaration may have become definition so check again.
- const VarDecl *DeclToCheck = VD->getDefinition();
- if (DeclToCheck)
- return !SemaRef->ShouldWarnIfUnusedFileScopedDecl(DeclToCheck);
-
- // Later redecls may add new information resulting in not having to warn,
- // so check again.
- DeclToCheck = VD->getMostRecentDecl();
- if (DeclToCheck != VD)
- return !SemaRef->ShouldWarnIfUnusedFileScopedDecl(DeclToCheck);
- }
-
- return false;
-}
-
/// Obtains a sorted list of functions that are undefined but ODR-used.
void Sema::getUndefinedButUsed(
SmallVectorImpl<std::pair<NamedDecl *, SourceLocation> > &Undefined) {
@@ -575,6 +528,62 @@
return Complete;
}
+/// \brief Used to prune the decls of Sema's UnusedFileScopedDecls vector.
+static bool ShouldRemoveFromUnused(Sema *SemaRef, const DeclaratorDecl *D,
+ RecordCompleteMap &RecordsComplete,
+ RecordCompleteMap &MNCComplete) {
+ if (D->getMostRecentDecl()->isUsed())
+ return true;
+
+ if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(D)) {
+ if (MD->getAccess() == AS_private &&
+ IsRecordFullyDefined(MD->getParent(), RecordsComplete, MNCComplete))
+ return false;
+ }
+
+ if (D->isExternallyVisible())
+ return true;
+
+ if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
+ // UnusedFileScopedDecls stores the first declaration.
+ // The declaration may have become definition so check again.
+ const FunctionDecl *DeclToCheck;
+ if (FD->hasBody(DeclToCheck))
+ return !SemaRef->ShouldWarnIfUnusedFileScopedDecl(DeclToCheck);
+
+ // Later redecls may add new information resulting in not having to warn,
+ // so check again.
+ DeclToCheck = FD->getMostRecentDecl();
+ if (DeclToCheck != FD)
+ return !SemaRef->ShouldWarnIfUnusedFileScopedDecl(DeclToCheck);
+ }
+
+ if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {
+ // If a variable usable in constant expressions is referenced,
+ // don't warn if it isn't used: if the value of a variable is required
+ // for the computation of a constant expression, it doesn't make sense to
+ // warn even if the variable isn't odr-used. (isReferenced doesn't
+ // precisely reflect that, but it's a decent approximation.)
+ if (VD->isReferenced() &&
+ VD->isUsableInConstantExpressions(SemaRef->Context))
+ return true;
+
+ // UnusedFileScopedDecls stores the first declaration.
+ // The declaration may have become definition so check again.
+ const VarDecl *DeclToCheck = VD->getDefinition();
+ if (DeclToCheck)
+ return !SemaRef->ShouldWarnIfUnusedFileScopedDecl(DeclToCheck);
+
+ // Later redecls may add new information resulting in not having to warn,
+ // so check again.
+ DeclToCheck = VD->getMostRecentDecl();
+ if (DeclToCheck != VD)
+ return !SemaRef->ShouldWarnIfUnusedFileScopedDecl(DeclToCheck);
+ }
+
+ return false;
+}
+
/// ActOnEndOfTranslationUnit - This is called at the very end of the
/// translation unit when EOF is reached and all but the top-level scope is
/// popped.
@@ -638,13 +647,6 @@
assert(DelayedDefaultedMemberExceptionSpecs.empty());
assert(DelayedDestructorExceptionSpecChecks.empty());
- // Remove file scoped decls that turned out to be used.
- UnusedFileScopedDecls.erase(
- std::remove_if(UnusedFileScopedDecls.begin(0, true),
- UnusedFileScopedDecls.end(),
- std::bind1st(std::ptr_fun(ShouldRemoveFromUnused), this)),
- UnusedFileScopedDecls.end());
-
if (TUKind == TU_Prefix) {
// Translation unit prefixes don't need any of the checking below.
TUScope = 0;
@@ -748,14 +750,20 @@
}
+ RecordCompleteMap RecordsComplete;
+ RecordCompleteMap MNCComplete;
+
// If there were errors, disable 'unused' warnings since they will mostly be
// noise.
if (!Diags.hasErrorOccurred()) {
+ llvm::SmallSet<const Decl*, 16> SeenDecls;
+
// Output warning for unused file scoped decls.
for (UnusedFileScopedDeclsType::iterator
I = UnusedFileScopedDecls.begin(ExternalSource),
E = UnusedFileScopedDecls.end(); I != E; ++I) {
- if (ShouldRemoveFromUnused(this, *I))
+ if (ShouldRemoveFromUnused(this, *I, RecordsComplete, MNCComplete) ||
+ !SeenDecls.insert(*I))
continue;
if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(*I)) {
@@ -810,8 +818,6 @@
if (Diags.getDiagnosticLevel(diag::warn_unused_private_field,
SourceLocation())
!= DiagnosticsEngine::Ignored) {
- RecordCompleteMap RecordsComplete;
- RecordCompleteMap MNCComplete;
for (NamedDeclSetType::iterator I = UnusedPrivateFields.begin(),
E = UnusedPrivateFields.end(); I != E; ++I) {
const NamedDecl *D = *I;
Index: lib/Sema/SemaDecl.cpp
===================================================================
--- lib/Sema/SemaDecl.cpp (revision 206567)
+++ lib/Sema/SemaDecl.cpp (working copy)
@@ -1216,6 +1216,9 @@
if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(FD)) {
if (MD->isVirtual() || IsDisallowedCopyOrAssign(MD))
return false;
+
+ if (MD->getAccess() == AS_private)
+ return true;
} else {
// 'static inline' functions are defined in headers; don't warn.
if (FD->isInlineSpecified() &&
Index: lib/Sema/SemaDeclCXX.cpp
===================================================================
--- lib/Sema/SemaDeclCXX.cpp (revision 206567)
+++ lib/Sema/SemaDeclCXX.cpp (working copy)
@@ -2129,6 +2129,9 @@
}
}
+ if (DeclaratorDecl *D = dyn_cast<DeclaratorDecl>(Member))
+ MarkUnusedFileScopedDecl(D);
+
return Member;
}
Index: test/SemaCXX/warn-unused-filescoped.cpp
===================================================================
--- test/SemaCXX/warn-unused-filescoped.cpp (revision 206567)
+++ test/SemaCXX/warn-unused-filescoped.cpp (working copy)
@@ -193,4 +193,31 @@
static void func() {}
}
+
+class c9 {
+ void f() {} // expected-warning {{unused member function}}
+
+public:
+ void a() {}
+};
+
+
+class c10 {
+ void f() {} // no warning
+
+public:
+ void a();
+};
+
+
+// FIXME: this requires computing the CFG
+class c11 {
+ void f(); // FIXME-warning {{unused member function}}
+
+public:
+ void a();
+};
+
+void c11::a() {}
+
#endif
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
