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

[Richard Smith]

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.

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

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

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

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

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