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.cs.uiuc.edu/pipermail/cfe-commits/attachments/20120509/e6d9cc56/attachment-0001.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clangpatch
Type: application/octet-stream
Size: 13061 bytes
Desc: not available
Url : http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20120509/e6d9cc56/attachment-0001.obj