On Feb 6, 2012, at 2:04 AM, Daniel Jasper wrote:
> Comments inline, new version attached.
> [snip]
A few comments…
+ 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?
+ 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()) {
+ 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.
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.
+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." Note that there's a C++11 case missing
here, for fields with an initializer
struct X {
int i = 7;
};
+ 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.
+ 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.
--- 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?
+ // 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?
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?
- Doug
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
