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

[Daniel Jasper]

On Mon, Jun 4, 2012 at 8:33 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> I have a few comments in addition to Jordy's:
>
> * Since pure virtual functions can be defined, the diagnostic as it
> stands might have false positives. In general, I think that's
> unavoidable and acceptable, but in the specific case of a pure virtual
> destructor, I think we should require that the TU contains a
> definition: such a destructor must actually be defined for the class
> to be useful as a base class, and may well be the only member of the
> class which has an out-of-line definition, and might reasonably
> contain the only use (other than a side-effect-free initialization) of
> a private member.
>

Do you mean that we should require that as part of this warning (i.e. mark
those classes as incomplete) or do you mean we should create a separate
warning for that? Also, do you think such a destructor might actually have
side-effects based on the otherwise unused fields. I somewhat doubt that as
we are only looking at pointers or primitive fields.. We might have
something like an owning pointer, but there the destructor will never be
the only "use".


> * The check for isTemplateDecl() in IsRecordFullyDefined is redundant,
> since CXXRecordDecls are never TemplateDecls.


 Removed.


> * It looks like in-class initializers and (in-constructor) member
> initializers are handled slightly differently (particularly for
> non-class types where the initializer has side-effects). It would seem
> more consistent with the language semantics if in-class initializers
> were treated exactly like member initializers, and only considered if
> they're actually used by a constructor. [... it also strikes me that a
> warning for an unused in-class initializer could be useful :) ...]
>

Just to clarify: An in-class initializer is only used, if at least one
constructor does not have a corresponding member initializer?

* I would like to see some test cases for unions, and for anonymous
> struct/union members, in particular for cases like this:
>
>  class S {
>    // ... no use of Aligner ...
>  private:
>    union {
>      void *Aligner;
>      unsigned char Data[8];
>    };
>  };
>

I'd like to get the first version in. Thus I'd like to add this test case
together with a FIXME. The correct solution for this (and especially for
anonymous structs) would be to look at the corresponding DeclContext, right?


> (See clang::DependentFunctionTempateSpecializationInfo for a
> real-world example of this.) It looks like we won't diagnose that case
> (because Aligner is technically public, despite being effectively a
> private member of S). But I suspect we will diagnose this (and
> probably shouldn't):
>
> union S {
>  // ... no use of Aligner ...
> private:
>  void *Aligner;
>  unsigned char Data[8];
> };
>
> Perhaps we should conservatively disable the diagnostic for union members?
>

Why should we not warn in this case? I think in general we might want to
exempt char-arrays as they are commonly use for alignment, but other than
that?

Daniel


> Thanks!
> Richard
>
> On Mon, Jun 4, 2012 at 10:22 AM, Jordan Rose <jordan_rose at apple.com>
> wrote:
> >
> > On Jun 4, 2012, at 1:43 , Daniel Jasper <djasper at google.com> wrote:
> >> Thanks for coming up with this!! I propose a different solution which
> utilizes the fact that friend relationships are not transitive. Thus, if a
> class has a friend, it only needs to check whether all methods and nested
> classes of the friend are defined but does not need to consider the
> friend's friends.
> >
> > Oh, of course! Good catch.
> >
> >> Could you take one last look as the implementation of
> IsRecordFullyDefined has changed substantially?
> >
> > Thank you for being careful. :-) Only small comments this time, I think:
> >
> > - Please attach the & to the variable name for references, per LLVM
> style conventions (same as pointers).
> >
> > - You've got an isa followed by dyn_cast for CXXMethodDecl; please
> change to just dyn_cast (like CXXRecordDecl).
> >
> > - DenseMap has an operator[], which is a little easier to read than
> insert+make_pair.
> >
> > - This is very nitpicky, but the style guide says to put all comments on
> their own line, and to end them with periods (full stops). So "// This is a
> template friend, give up" should be reformatted as such.
> >
> > Also, it took me a while to convince myself that CXXRecordDecls coming
> out of getFriendDecl() are also problematic, but what convinced me is that
> if they don't have a TypeSourceInfo, we can't possibly see their
> definition. It does seem like it is possible to have CXXRecordDecls come
> out of getFriendDecl(), though, and that might be worth commenting.
> >
> > I'm tempted to say that passing around a pair of maps to two different
> methods is worth a private class, but I guess it's fine the way it is.
> >
> > I think that's it. Good work!
> >
> > Jordan
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120604/32c9dab4/attachment.html>