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

[Chandler Carruth]

Thanks, comments inline... This is sadly a pretty cursory read, sorry,
working on other stuff. I think, especially given the "used" concept at
play here, Richard or Eli looking at this would be very good. =] Thanks
again for working on it.

--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -134,6 +134,9 @@  def warn_unneeded_member_function : Warning<
   "member function %0 is not needed and will not be emitted">,
   InGroup<UnneededMemberFunction>, DefaultIgnore;

+def warn_unused_private_field: Warning<"private field %0 is not used">,
+  InGroup<UnusedPrivateField>, DefaultIgnore;
+

I wouldn't separate this in its own group w/ vertical whitespace, but
rather jam it in with the other unused warning messages.

--- a/lib/Sema/SemaDeclCXX.cpp
+++ b/lib/Sema/SemaDeclCXX.cpp
@@ -1617,8 +1617,25 @@  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);
+    if (FD->getAccess() == AS_private) {
+      bool hasNoSideEffects = true;
+      if (!FD->getType().isNull()) {
+        if (const CXXRecordDecl *RD =
+            FD->getType().getTypePtrOrNull()->getAsCXXRecordDecl()) {

You shouldn't need getTypePtrOrNull() here. QualType provides an ->
overload that delegates to Type.

+      if
(!FD->getParent()->getTypeForDecl()->isInstantiationDependentType() &&
+          hasNoSideEffects)

I'm a bit surprised at checking InstantiationDependentType here... Not just
Dependent... Also some comments as to what this is supposed to handle would
be good...

@@ -2063,6 +2080,15 @@  Sema::BuildMemberInitializer(ValueDecl *Member,
   assert((DirectMember || IndirectMember) &&
          "Member must be a FieldDecl or IndirectFieldDecl");

+  // FIXME: Allow more initializers not to count as usage, e.g. where all
+  // arguments are primitive types.
+  ValueDecl *VD = dyn_cast<ValueDecl>(Member);

If you know the cast will always succeed, just use "cast" rather than
"dyn_cast".

+  if (!VD->getType()->isBuiltinType() &&
+      !VD->getType()->isAnyPointerType() &&

Why are these here? We already have constraints on the type of things above
when we check the Field Decl? Maybe I just don't understand what these are
doing.

--- a/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/lib/Sema/SemaTemplateInstantiate.cpp
@@ -1791,6 +1791,19 @@  Sema::InstantiateClass(SourceLocation
PointOfInstantiation,
         if (OldField->getInClassInitializer())
           FieldsWithMemberInitializers.push_back(std::make_pair(OldField,
                                                                 Field));
+        if (Field->getAccess() == AS_private) {
+          bool hasNoSideEffects = true;
+          if (!Field->getType().isNull()) {
+            if (const CXXRecordDecl *RD =
+                Field->getType().getTypePtrOrNull()->getAsCXXRecordDecl())
{
+              hasNoSideEffects = RD->isCompleteDefinition() &&
+                                 RD->hasTrivialDefaultConstructor() &&
+                                 RD->hasTrivialDestructor();
+            }
+          }
+          if (hasNoSideEffects)
+            UnusedPrivateFields.insert(Field);
+        }

This is the second copy of this pattern. We should either have template
instantiation call the not-template Sema routines to do this, or we should
factor the predicate into a helper somewhere, likely FieldDecl.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120201/dda3ba39/attachment.html>