[cfe-commits] Clang change to detect and warn about unused private members

Douglas Gregor dgregor at apple.com
Sat Mar 17 00:30:57 PDT 2012


On Feb 6, 2012, at 2:04 AM, Daniel Jasper wrote:

> Comments inline, new version attached.
> [snip]

A few comments…


+  typedef llvm::SmallPtrSet<const NamedDecl*, 16> NamedDeclSetType;
+
+  /// \brief Set containing all declared private fields that are not used.
+  NamedDeclSetType UnusedPrivateFields;

How big does this set tend to get with, say, a typical .cpp file in LLVM or Clang? Big enough that we should be concerned about a performance impact?

+      if (TypeSourceInfo *TSI = (*I)->getFriendType()) {
+        if (const Type *T = TSI->getType().getTypePtrOrNull()) {

With a non-NULL TSI, TSI->getType() will always return a non-NULL QualType. You can just collapse this into the next line to get

	if (CXXRecordDecl *FriendDecl = TSI->getType()->getAsCXXRecordDecl()) {

+        if (const FunctionDecl *FD =
+            dyn_cast_or_null<FunctionDecl>((*I)->getFriendDecl()))
+          Complete = FD->isDefined();

I don't think the "_or_null" is needed here, because a FriendDecl either befriends a type or a decl.

I note that these three loops:

+    for (CXXRecordDecl::friend_iterator I = RD->friend_begin(),
+                                        E = RD->friend_end();
+         I != E && Complete; ++I) {

+    for (CXXRecordDecl::method_iterator M = RD->method_begin(),
+                                        E = RD->method_end();
+         M != E && Complete; ++M) {

+    for (record_iterator I(RD->decls_begin()), E(RD->decls_begin());
+         I != E && Complete; ++I) {

All make a complete pass over all of the declarations within each CXXRecordDecl, which is rather inefficient. It would be better to make one pass over decls_begin/decls_end and deal with all of the potential cases.

+bool FieldDecl::hasSideEffects() const {
+  if (!getType().isNull()) {
+    if (const CXXRecordDecl *RD = getType()->getAsCXXRecordDecl()) {
+      return !RD->isCompleteDefinition() ||
+             !RD->hasTrivialDefaultConstructor() ||
+             !RD->hasTrivialDestructor();
+    }
+  }
+  return false;
+}

I don't think that hasSideEffects() belongs on FieldDecl. It can be some kind of Sema utility routine, perhaps, that makes it obvious that we mean "no side effects due to its initialization." Note that there's a C++11 case missing here, for fields with an initializer

	struct X {
		int i = 7;
	};



+  llvm::SmallPtrSet<const CXXRecordDecl*, 16> NotFullyDefined;
+  llvm::SmallPtrSet<const CXXRecordDecl*, 16> FullyDefined;
+  for (NamedDeclSetType::iterator I = UnusedPrivateFields.begin(),
+                                  E = UnusedPrivateFields.end(); I != E; ++I) {

This will give a non-deterministic ordering of warnings. You probably want to use a SetVector here to keep things in order.

+    const NamedDecl *D = *I;
+    const CXXRecordDecl *RD = cast<const CXXRecordDecl>(D->getDeclContext());
+    if (RD && IsRecordFullyDefined(RD, NotFullyDefined,
+                                                    FullyDefined)) {
+      Diag(D->getLocation(), diag::warn_unused_private_field)
+          << D->getNameAsString();

There's no need for D->getNameAsString(). You can just use D->getDeclName() and the diagnostic system will format it.

--- a/lib/Sema/SemaDeclCXX.cpp
+++ b/lib/Sema/SemaDeclCXX.cpp
@@ -1643,8 +1643,17 @@ Sema::ActOnCXXMemberDeclarator(Scope *S, AccessSpecifier AS, Declarator &D,
 
   assert((Name || isInstField) && "No identifier for non-field ?");
 
-  if (isInstField)
-    FieldCollector->Add(cast<FieldDecl>(Member));
+  if (isInstField) {
+    FieldDecl *FD = cast<FieldDecl>(Member);
+    FieldCollector->Add(FD);
+    // Remember all private FieldDecls that have no side effects and are not
+    // part of a dependent type declaration.
+    if (FD->getAccess() == AS_private &&
+        !FD->getParent()->getTypeForDecl()->isDependentType() &&
+        !FD->hasSideEffects())
+      UnusedPrivateFields.insert(FD);
+  }
+

Shouldn't you also check that FD actually has a name?

+  // Mark FieldDecl as being used if it is a non-primitive type and the
+  // initializer does not call the default constructor (which is trivial
+  // for all entries in UnusedPrivateFields).
+  // FIXME: Make this smarter once more side effect-free types can be
+  // determined.
+  ValueDecl *VD = cast<ValueDecl>(Member);
+  if (!VD->getType()->isBuiltinType() &&
+      !VD->getType()->isAnyPointerType() &&
+      Args.begin() != Args.end()) {
+    UnusedPrivateFields.erase(VD);
+  }
+

I find the check for pointer + built-in types odd. Isn't there a more obvious, existing type predicate that checks what you want?

diff --git a/lib/Sema/SemaTemplateInstantiate.cpp b/lib/Sema/SemaTemplateInstantiate.cpp
index dde7826..f30a521 100644
--- a/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/lib/Sema/SemaTemplateInstantiate.cpp
@@ -1809,6 +1809,9 @@ Sema::InstantiateClass(SourceLocation PointOfInstantiation,
         if (OldField->getInClassInitializer())
           FieldsWithMemberInitializers.push_back(std::make_pair(OldField,
                                                                 Field));
+        // Remember all private FieldDecls that have no side effects.
+        if (Field->getAccess() == AS_private && !Field->hasSideEffects())
+          UnusedPrivateFields.insert(Field);

Templates are an interesting case, because it's actually quite unlikely that we'll ever see the definition of all of the member functions of a class template, since no translation unit is likely to instantiate all of them. Perhaps we shouldn't bother even trying to make this warning work for templates?

	- Doug





More information about the cfe-commits mailing list