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

[Daniel Jasper]

On Sun, May 27, 2012 at 2:31 AM, Jordy Rose <jediknil at belkadan.com> wrote:

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


Done.


> +  // 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?
>

Removed VD.

I personally agree, but I have also heard that some people do initialize
fields with methods that have side effects and use the value only with
certain #ifdefs (e.g. for debugging). In these cases, the warning would be
annoying. Thus, in order to reduce false positives as far as possible in
this first version, I declared this as a "use". I am happy to change it,
though.

+      } 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.


Yes, you are right. I actually wanted to not check the injected class name
but I messed it up (and didn't find isInjectedClassname). Also, it did not
correctly handle cases where the inner class was defined outside of the
outer class. Fixed and tests added.


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

Done.


> 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?
>

I am now checking whether the diagnostic is enabled before inserting into
the set and before doing the checks at the end of the translation unit.
Checking before removing from the (then empty) set probably does not give
any benefit. I am not sure whether the check on the initializer arguments
is expensive (especially the call to HasSideEffects). What do you think?


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

I added a static private member to the test. The warning does not fire at
the moment and I think that is ok for the first version.


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


Yes, as of today :-). Please find a new version of the patch attached.

Daniel


>
> 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?
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120530/05dfa2b3/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clangpatch
Type: application/octet-stream
Size: 13486 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120530/05dfa2b3/attachment.obj>