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

[Jordy Rose]

I guess I'm okay with the extra memory usage. A few last comments:

+bool InitializationHasSideEffects(const FieldDecl& FD) {

Please make this file-scoped (static).

+  // 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.
+  if (NumArgs > 0) {
+    ValueDecl *VD = cast<ValueDecl>(Member);
+    if (VD->getType()->isRecordType()) {
+      UnusedPrivateFields.remove(VD);
+    } else {
+      for (unsigned i = 0; i < NumArgs; ++i) {
+        if (Args[i]->HasSideEffects(Context)) {
+          UnusedPrivateFields.remove(VD);
+          break;
+        }
+      }
+    }
+  }

'Member' is already typed as a ValueDecl. Also, even if one of the constructor arguments to a primitive type has side effects, shouldn't the field still count as unused if it's never referenced again?

+      } else if (const CXXRecordDecl *R = dyn_cast<CXXRecordDecl>(*I)) {
+        // Check if nested classes are complete.
+        if (R->hasDefinition() &&
+            R->getCanonicalDecl() != RD->getCanonicalDecl())
+          Complete = IsRecordFullyDefined(R, NotFullyDefined, FullyDefined);
+      }

This clause in IsRecordFullyDefined seems a little fishy to me. If a nested class does NOT have a definition, the enclosing class is still complete? If it has a definition but the canonical decl is the current class -- okay, I get that, if the class has a reference to itself.


+    const CXXRecordDecl *RD = cast<const CXXRecordDecl>(D->getDeclContext());
+    if (RD && IsRecordFullyDefined(RD, NotFullyDefined, FullyDefined)) {

This cast should be dyn_cast, and the 'const' is unnecessary.

Finally, even though there's not a big hit traversing the fields at the end, we probably still shouldn't pay for this if the diagnostic is off. Can you check for that before inserting into UnusedPrivateFields, or even doing all the tests?

(Oh, and how does this do with private static members? I think you'd be missing a case there. If you want to just exclude them for now, though, that's okay.)

Sorry for the review delay. Do you have commit access?

Jordy


On May 22, 2012, at 18:17, Daniel Jasper wrote:

> 
> 
> On Fri, May 11, 2012 at 12:37 AM, Jordan Rose <jediknil at belkadan.com> wrote:
> 
> On May 10, 2012, at 18:12, Daniel Jasper wrote:
> 
> > Ok, I now reuse Sema::RecordDeclSetTy.
> >
> > As for putting a bit into FieldDecl as opposed to the UnusedPrivateField SetVector: It seems like FieldDecl is carefully crafted for size. Just adding another bit for this use case doesn't seem right.
> >
> > Please find the new version attached.
> 
> CachedFieldIndex is just the index of the field in the record; there's no way that'll reach 2^31 anyway. (For comparison, C++11 [implimits] suggests a minimum of 4096 members.)
> 
> Also, it looks like Decl already has an isUsed() predicate. Are we already marking FieldDecls this way?
> 
> I have been looking into this some more. Marking the fields as used works fine, but I didn't find an easy way to iterate over all of the FieldDecls in a translation unit at the end. Thus, I suggest to stick with the previous version of my patch. Should also neither be a big performance nor memory impact. What do you think?