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

[Daniel Jasper]

Did/will anyone have time to take a look at this?

On Mon, Feb 6, 2012 at 11:04 AM, Daniel Jasper <djasper at google.com> wrote:

> Comments inline, new version attached.
>
> On Wed, Feb 1, 2012 at 10:52 AM, Chandler Carruth <chandlerc at google.com>wrote:
>
>> Thanks, comments inline... This is sadly a pretty cursory read, sorry,
>> working on other stuff. I think, especially given the "used" concept at
>> play here, Richard or Eli looking at this would be very good. =] Thanks
>> again for working on it.
>>
>> --- a/include/clang/Basic/DiagnosticSemaKinds.td
>> +++ b/include/clang/Basic/DiagnosticSemaKinds.td
>> @@ -134,6 +134,9 @@  def warn_unneeded_member_function : Warning<
>>    "member function %0 is not needed and will not be emitted">,
>>    InGroup<UnneededMemberFunction>, DefaultIgnore;
>>
>> +def warn_unused_private_field: Warning<"private field %0 is not used">,
>> +  InGroup<UnusedPrivateField>, DefaultIgnore;
>> +
>>
>> I wouldn't separate this in its own group w/ vertical whitespace, but
>> rather jam it in with the other unused warning messages.
>>
>
> Done.
>
>
>> --- a/lib/Sema/SemaDeclCXX.cpp
>> +++ b/lib/Sema/SemaDeclCXX.cpp
>> @@ -1617,8 +1617,25 @@  Sema::ActOnCXXMemberDeclarator(Scope *S,
>> AccessSpecifier AS, Declarator &D,
>>
>>    assert((Name || isInstField) && "No identifier for non-field ?");
>>
>> -  if (isInstField)
>> -    FieldCollector->Add(cast<FieldDecl>(Member));
>> +  if (isInstField) {
>> +    FieldDecl *FD = cast<FieldDecl>(Member);
>> +    FieldCollector->Add(FD);
>> +    if (FD->getAccess() == AS_private) {
>> +      bool hasNoSideEffects = true;
>> +      if (!FD->getType().isNull()) {
>> +        if (const CXXRecordDecl *RD =
>> +            FD->getType().getTypePtrOrNull()->getAsCXXRecordDecl()) {
>>
>> You shouldn't need getTypePtrOrNull() here. QualType provides an ->
>> overload that delegates to Type.
>>
>
> Done.
>
>
>>
>> +      if
>> (!FD->getParent()->getTypeForDecl()->isInstantiationDependentType() &&
>> +          hasNoSideEffects)
>>
>> I'm a bit surprised at checking InstantiationDependentType here... Not
>> just Dependent... Also some comments as to what this is supposed to handle
>> would be good...
>>
>
> Done.
>
>
>> @@ -2063,6 +2080,15 @@  Sema::BuildMemberInitializer(ValueDecl *Member,
>>    assert((DirectMember || IndirectMember) &&
>>           "Member must be a FieldDecl or IndirectFieldDecl");
>>
>> +  // FIXME: Allow more initializers not to count as usage, e.g. where all
>> +  // arguments are primitive types.
>> +  ValueDecl *VD = dyn_cast<ValueDecl>(Member);
>>
>> If you know the cast will always succeed, just use "cast" rather than
>> "dyn_cast".
>>
>
> Done.
>
>
>> +  if (!VD->getType()->isBuiltinType() &&
>> +      !VD->getType()->isAnyPointerType() &&
>>
>> Why are these here? We already have constraints on the type of things
>> above when we check the Field Decl? Maybe I just don't understand what
>> these are doing.
>>
>
> I added a comment. Basically, this currently marks fields of a
> non-primitive type as used, if a non-trivial constructor is called by an
> initializer.
>
> --- a/lib/Sema/SemaTemplateInstantiate.cpp
>> +++ b/lib/Sema/SemaTemplateInstantiate.cpp
>> @@ -1791,6 +1791,19 @@  Sema::InstantiateClass(SourceLocation
>> PointOfInstantiation,
>>          if (OldField->getInClassInitializer())
>>            FieldsWithMemberInitializers.push_back(std::make_pair(OldField,
>>                                                                  Field));
>> +        if (Field->getAccess() == AS_private) {
>> +          bool hasNoSideEffects = true;
>> +          if (!Field->getType().isNull()) {
>> +            if (const CXXRecordDecl *RD =
>> +
>>  Field->getType().getTypePtrOrNull()->getAsCXXRecordDecl()) {
>> +              hasNoSideEffects = RD->isCompleteDefinition() &&
>> +                                 RD->hasTrivialDefaultConstructor() &&
>> +                                 RD->hasTrivialDestructor();
>> +            }
>> +          }
>> +          if (hasNoSideEffects)
>> +            UnusedPrivateFields.insert(Field);
>> +        }
>>
>> This is the second copy of this pattern. We should either have template
>> instantiation call the not-template Sema routines to do this, or we should
>> factor the predicate into a helper somewhere, likely FieldDecl.
>>
>
> Done. I implemented a method hasSideEffects that computes whether the
> declaration of the field has side effects. It currently only recognizes the
> trivial default constructor and the trivial destructor as side-effect-free.
> This can be extended in the future. Should the method be moved to
> DeclaratorDecl or ValueDecl (it doesn't use anything FieldDecl-specific)?
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120316/84078084/attachment.html>