Finally, a new version of this patch.<br><br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ typedef llvm::SmallPtrSet<const NamedDecl*, 16> NamedDeclSetType;<br>
+<br>
+ /// \brief Set containing all declared private fields that are not used.<br>
+ NamedDeclSetType UnusedPrivateFields;<br>
<br>
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?<br></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ if (TypeSourceInfo *TSI = (*I)->getFriendType()) {<br>
+ if (const Type *T = TSI->getType().getTypePtrOrNull()) {<br>
<br>
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<br>
<br>
if (CXXRecordDecl *FriendDecl = TSI->getType()->getAsCXXRecordDecl()) {<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ if (const FunctionDecl *FD =<br>
+ dyn_cast_or_null<FunctionDecl>((*I)->getFriendDecl()))<br>
+ Complete = FD->isDefined();<br>
<br>
I don't think the "_or_null" is needed here, because a FriendDecl either befriends a type or a decl.<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I note that these three loops:<br>
<br>
+ for (CXXRecordDecl::friend_iterator I = RD->friend_begin(),<br>
+ E = RD->friend_end();<br>
+ I != E && Complete; ++I) {<br>
<br>
+ for (CXXRecordDecl::method_iterator M = RD->method_begin(),<br>
+ E = RD->method_end();<br>
+ M != E && Complete; ++M) {<br>
<br>
+ for (record_iterator I(RD->decls_begin()), E(RD->decls_begin());<br>
+ I != E && Complete; ++I) {<br>
<br>
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.<br></blockquote>
<div><br></div><div>Done. </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+bool FieldDecl::hasSideEffects() const {<br>
+ if (!getType().isNull()) {<br>
+ if (const CXXRecordDecl *RD = getType()->getAsCXXRecordDecl()) {<br>
+ return !RD->isCompleteDefinition() ||<br>
+ !RD->hasTrivialDefaultConstructor() ||<br>
+ !RD->hasTrivialDestructor();<br>
+ }<br>
+ }<br>
+ return false;<br>
+}<br>
<br>
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." </blockquote><div>
<br></div><div>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).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Note that there's a C++11 case missing here, for fields with an initializer<br>
<br>
struct X {<br>
int i = 7;<br>
};<br>
<br></blockquote><div><br></div><div>Added test and implementation for this.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ llvm::SmallPtrSet<const CXXRecordDecl*, 16> NotFullyDefined;<br>
+ llvm::SmallPtrSet<const CXXRecordDecl*, 16> FullyDefined;<br>
+ for (NamedDeclSetType::iterator I = UnusedPrivateFields.begin(),<br>
+ E = UnusedPrivateFields.end(); I != E; ++I) {<br>
<br>
This will give a non-deterministic ordering of warnings. You probably want to use a SetVector here to keep things in order.<br></blockquote><div><br>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ const NamedDecl *D = *I;<br>
+ const CXXRecordDecl *RD = cast<const CXXRecordDecl>(D->getDeclContext());<br>
+ if (RD && IsRecordFullyDefined(RD, NotFullyDefined,<br>
+ FullyDefined)) {<br>
+ Diag(D->getLocation(), diag::warn_unused_private_field)<br>
+ << D->getNameAsString();<br>
<br>
There's no need for D->getNameAsString(). You can just use D->getDeclName() and the diagnostic system will format it.<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
--- a/lib/Sema/SemaDeclCXX.cpp<br>
+++ b/lib/Sema/SemaDeclCXX.cpp<br>
@@ -<a href="tel:1643" value="+491643">1643</a>,8 +1643,17 @@ Sema::ActOnCXXMemberDeclarator(Scope *S, AccessSpecifier AS, Declarator &D,<br>
<div class="im"><br>
assert((Name || isInstField) && "No identifier for non-field ?");<br>
<br>
- if (isInstField)<br>
- FieldCollector->Add(cast<FieldDecl>(Member));<br>
+ if (isInstField) {<br>
+ FieldDecl *FD = cast<FieldDecl>(Member);<br>
+ FieldCollector->Add(FD);<br>
</div>+ // Remember all private FieldDecls that have no side effects and are not<br>
+ // part of a dependent type declaration.<br>
+ if (FD->getAccess() == AS_private &&<br>
+ !FD->getParent()->getTypeForDecl()->isDependentType() &&<br>
+ !FD->hasSideEffects())<br>
+ UnusedPrivateFields.insert(FD);<br>
+ }<br>
+<br>
<br>
Shouldn't you also check that FD actually has a name?<br></blockquote><div><br></div><div>Yes. Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ // Mark FieldDecl as being used if it is a non-primitive type and the<br>
+ // initializer does not call the default constructor (which is trivial<br>
+ // for all entries in UnusedPrivateFields).<br>
+ // FIXME: Make this smarter once more side effect-free types can be<br>
+ // determined.<br>
+ ValueDecl *VD = cast<ValueDecl>(Member);<br>
<div class="im">+ if (!VD->getType()->isBuiltinType() &&<br>
+ !VD->getType()->isAnyPointerType() &&<br>
</div>+ Args.begin() != Args.end()) {<br>
+ UnusedPrivateFields.erase(VD);<br>
+ }<br>
+<br>
<br>
I find the check for pointer + built-in types odd. Isn't there a more obvious, existing type predicate that checks what you want?<br></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
diff --git a/lib/Sema/SemaTemplateInstantiate.cpp b/lib/Sema/SemaTemplateInstantiate.cpp<br>
index dde7826..f30a521 100644<br>
--- a/lib/Sema/SemaTemplateInstantiate.cpp<br>
+++ b/lib/Sema/SemaTemplateInstantiate.cpp<br>
@@ -1809,6 +1809,9 @@ Sema::InstantiateClass(SourceLocation PointOfInstantiation,<br>
<div class="im"> if (OldField->getInClassInitializer())<br>
FieldsWithMemberInitializers.push_back(std::make_pair(OldField,<br>
Field));<br>
</div>+ // Remember all private FieldDecls that have no side effects.<br>
+ if (Field->getAccess() == AS_private && !Field->hasSideEffects())<br>
+ UnusedPrivateFields.insert(Field);<br>
<br>
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?<br>
</blockquote><div><br></div><div>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.</div><div><br></div><div>Kind regards,<br>
Daniel</div></div>