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

[Daniel Jasper]

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

> On Mon, Jun 4, 2012 at 12:36 PM, Daniel Jasper <djasper at google.com> wrote:
> > 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?
>
> I mean that we should not consider a class to be completely defined if
> it has a pure virtual destructor with no definition in this TU.
>

Done and test added.

 > 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".
>
> I'm imagining a case like this:
>
> // .h
> class RAIIBase {
> public:
>  RAIIBase(const char *p) : LogMessage(p) {
>    my_debug_stream << "Creating " << p;
>  }
>  virtual ~RAIIBase() = 0;
> private:
>  const char *LogMessage;
> };
>
> // .cc
> RAIIBase::~RAIIBase() {
>  my_debug_stream << "Destroying " << LogMessage;
> }
>

I wonder, whether this is a case that really ever occurs (due to multiple
rare things being used at the same time). But ok.


> If I've understood correctly, the initialization of LogMessage won't
> count as a 'use' because it has no side-effects, so any TU including
> the .h file (other than the corresponding .cc file) would flag
> 'LogMessage' as being unused.
>
> >> * 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?
>
> There are some additional complications if the class is a union or has
> an anonymous union member. Perhaps moving the marking-as-used for
> in-class initializers to CollectFieldInitializer would be best.
>

Moved this to CollectFieldInitializer. Added call to HasSideEffects (+test)
for symmetry with constructor initializers.

>
> >> * 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?
>
> Yes, to find the outer struct, you can walk through the parent
> DeclContexts (while they're RecordDecls and
> RecordDecl::isAnonymousStructOrUnion returns true).


Ok, I will do this once the first patch is in as I am not overly concerned
about this kind of false negative.


>  >> (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?
>
> Aligner is 'unused', but has a semantic effect. Note that in the
> clang::DependentFunctionTempateSpecializationInfo case, there is no
> char array. I'm concerned that a warning for cases like this would
> have a high false positive rate. (I have the same concern for padding
> members in structs, but I would expect those to generally be public,
> since such structs are usually intended to be PODs.)
>

Ah, (things-I-know-about-C++)++. :-)
I know exclude unions from this analysis.

New patch attached.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120605/13a41c0d/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: unused-private-field.patch
Type: application/octet-stream
Size: 16102 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120605/13a41c0d/attachment.obj>