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

[Daniel Jasper]

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/20120206/5ab1c413/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clangpatch06022012
Type: application/octet-stream
Size: 14133 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120206/5ab1c413/attachment.obj>