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

[Richard Smith]

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.

> 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;
}

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.

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

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