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

[Daniel Jasper]

Finally, a new version of this patch.

+  typedef llvm::SmallPtrSet<const NamedDecl*, 16> NamedDeclSetType;
> +
> +  /// \brief Set containing all declared private fields that are not used.
> +  NamedDeclSetType UnusedPrivateFields;
>
> How big does this set tend to get with, say, a typical .cpp file in LLVM
> or Clang? Big enough that we should be concerned about a performance impact?
>

While compiling files from Sema, I got the following statistics: The
maximum size of this structure was 560, with 1600 total inserts and 14000
attempted deletes. This should not be a performance impact, but I don't
really know with respect to LLVM/Clang.


> +      if (TypeSourceInfo *TSI = (*I)->getFriendType()) {
> +        if (const Type *T = TSI->getType().getTypePtrOrNull()) {
>
> With a non-NULL TSI, TSI->getType() will always return a non-NULL
> QualType. You can just collapse this into the next line to get
>
>        if (CXXRecordDecl *FriendDecl =
> TSI->getType()->getAsCXXRecordDecl()) {
>

Done.


> +        if (const FunctionDecl *FD =
> +            dyn_cast_or_null<FunctionDecl>((*I)->getFriendDecl()))
> +          Complete = FD->isDefined();
>
> I don't think the "_or_null" is needed here, because a FriendDecl either
> befriends a type or a decl.
>

Done.


> I note that these three loops:
>
> +    for (CXXRecordDecl::friend_iterator I = RD->friend_begin(),
> +                                        E = RD->friend_end();
> +         I != E && Complete; ++I) {
>
> +    for (CXXRecordDecl::method_iterator M = RD->method_begin(),
> +                                        E = RD->method_end();
> +         M != E && Complete; ++M) {
>
> +    for (record_iterator I(RD->decls_begin()), E(RD->decls_begin());
> +         I != E && Complete; ++I) {
>
> All make a complete pass over all of the declarations within each
> CXXRecordDecl, which is rather inefficient. It would be better to make one
> pass over decls_begin/decls_end and deal with all of the potential cases.
>

Done.

+bool FieldDecl::hasSideEffects() const {
> +  if (!getType().isNull()) {
> +    if (const CXXRecordDecl *RD = getType()->getAsCXXRecordDecl()) {
> +      return !RD->isCompleteDefinition() ||
> +             !RD->hasTrivialDefaultConstructor() ||
> +             !RD->hasTrivialDestructor();
> +    }
> +  }
> +  return false;
> +}
>
> I don't think that hasSideEffects() belongs on FieldDecl. It can be some
> kind of Sema utility routine, perhaps, that makes it obvious that we mean
> "no side effects due to its initialization."


I have now made it a local function in SemaDeclCXX.cpp and named it more
appropriately. It is not reused for templates anymore (see below).


> Note that there's a C++11 case missing here, for fields with an initializer
>
>        struct X {
>                int i = 7;
>        };
>
>
Added test and implementation for this.


> +  llvm::SmallPtrSet<const CXXRecordDecl*, 16> NotFullyDefined;
> +  llvm::SmallPtrSet<const CXXRecordDecl*, 16> FullyDefined;
> +  for (NamedDeclSetType::iterator I = UnusedPrivateFields.begin(),
> +                                  E = UnusedPrivateFields.end(); I != E;
> ++I) {
>
> This will give a non-deterministic ordering of warnings. You probably want
> to use a SetVector here to keep things in order.
>

Done.


> +    const NamedDecl *D = *I;
> +    const CXXRecordDecl *RD = cast<const
> CXXRecordDecl>(D->getDeclContext());
> +    if (RD && IsRecordFullyDefined(RD, NotFullyDefined,
> +                                                    FullyDefined)) {
> +      Diag(D->getLocation(), diag::warn_unused_private_field)
> +          << D->getNameAsString();
>
> There's no need for D->getNameAsString(). You can just use
> D->getDeclName() and the diagnostic system will format it.
>

Done.


> --- a/lib/Sema/SemaDeclCXX.cpp
> +++ b/lib/Sema/SemaDeclCXX.cpp
> @@ -1643,8 +1643,17 @@ 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);
> +    // Remember all private FieldDecls that have no side effects and are
> not
> +    // part of a dependent type declaration.
> +    if (FD->getAccess() == AS_private &&
> +        !FD->getParent()->getTypeForDecl()->isDependentType() &&
> +        !FD->hasSideEffects())
> +      UnusedPrivateFields.insert(FD);
> +  }
> +
>
> Shouldn't you also check that FD actually has a name?
>

Yes. Done.


> +  // Mark FieldDecl as being used if it is a non-primitive type and the
> +  // initializer does not call the default constructor (which is trivial
> +  // for all entries in UnusedPrivateFields).
> +  // FIXME: Make this smarter once more side effect-free types can be
> +  // determined.
> +  ValueDecl *VD = cast<ValueDecl>(Member);
> +  if (!VD->getType()->isBuiltinType() &&
> +      !VD->getType()->isAnyPointerType() &&
> +      Args.begin() != Args.end()) {
> +    UnusedPrivateFields.erase(VD);
> +  }
> +
>
> I find the check for pointer + built-in types odd. Isn't there a more
> obvious, existing type predicate that checks what you want?
>

Yes. I actually want to check that it is not a record-type. Done. Also, I
now check that the initializer itself does not have side-effects.


> diff --git a/lib/Sema/SemaTemplateInstantiate.cpp
> b/lib/Sema/SemaTemplateInstantiate.cpp
> index dde7826..f30a521 100644
> --- a/lib/Sema/SemaTemplateInstantiate.cpp
> +++ b/lib/Sema/SemaTemplateInstantiate.cpp
> @@ -1809,6 +1809,9 @@ Sema::InstantiateClass(SourceLocation
> PointOfInstantiation,
>          if (OldField->getInClassInitializer())
>           FieldsWithMemberInitializers.push_back(std::make_pair(OldField,
>                                                                 Field));
> +        // Remember all private FieldDecls that have no side effects.
> +        if (Field->getAccess() == AS_private && !Field->hasSideEffects())
> +          UnusedPrivateFields.insert(Field);
>
> Templates are an interesting case, because it's actually quite unlikely
> that we'll ever see the definition of all of the member functions of a
> class template, since no translation unit is likely to instantiate all of
> them. Perhaps we shouldn't bother even trying to make this warning work for
> templates?
>

Probably not as this cannot be solved locally for a TU. Another TU might
declare an explicit specialization using the otherwise unused private
member. Removed.

Kind regards,
Daniel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120509/e6d9cc56/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clangpatch
Type: application/octet-stream
Size: 13062 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120509/e6d9cc56/attachment.obj>