Did/will anyone have time to take a look at this?<br><br><div class="gmail_quote">On Mon, Feb 6, 2012 at 11:04 AM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com">djasper@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Comments inline, new version attached.<br><br><div class="gmail_quote"><div class="im">On Wed, Feb 1, 2012 at 10:52 AM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.<div>
<br></div><div><div>--- a/include/clang/Basic/DiagnosticSemaKinds.td</div><div>+++ b/include/clang/Basic/DiagnosticSemaKinds.td</div><div>@@ -134,6 +134,9 @@ def warn_unneeded_member_function : Warning<</div><div> "member function %0 is not needed and will not be emitted">,</div>
<div> InGroup<UnneededMemberFunction>, DefaultIgnore;</div><div> </div><div>+def warn_unused_private_field: Warning<"private field %0 is not used">,</div><div>+ InGroup<UnusedPrivateField>, DefaultIgnore;</div>
<div>+</div></div><div><br></div><div>I wouldn't separate this in its own group w/ vertical whitespace, but rather jam it in with the other unused warning messages.</div></blockquote><div> </div></div><div>Done.</div>
<div class="im"><div>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div></div><div><div>--- a/lib/Sema/SemaDeclCXX.cpp</div>
<div>+++ b/lib/Sema/SemaDeclCXX.cpp</div><div>@@ -1617,8 +1617,25 @@ Sema::ActOnCXXMemberDeclarator(Scope *S, AccessSpecifier AS, Declarator &D,</div><div> </div><div> assert((Name || isInstField) && "No identifier for non-field ?");</div>
<div> </div><div>- if (isInstField)</div><div>- FieldCollector->Add(cast<FieldDecl>(Member));</div><div>+ if (isInstField) {</div><div>+ FieldDecl *FD = cast<FieldDecl>(Member);</div><div>+ FieldCollector->Add(FD);</div>
<div>+ if (FD->getAccess() == AS_private) {</div><div>+ bool hasNoSideEffects = true;</div><div>+ if (!FD->getType().isNull()) {</div><div>+ if (const CXXRecordDecl *RD =</div><div>+ FD->getType().getTypePtrOrNull()->getAsCXXRecordDecl()) {</div>
</div><div><br></div><div>You shouldn't need getTypePtrOrNull() here. QualType provides an -> overload that delegates to Type.</div></blockquote><div><br></div></div><div>Done.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br></div><div><div>+ if (!FD->getParent()->getTypeForDecl()->isInstantiationDependentType() &&</div>
<div>+ hasNoSideEffects)</div></div><div><br></div><div>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...</div>
</blockquote><div><br></div></div><div>Done.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>@@ -2063,6 +2080,15 @@ Sema::BuildMemberInitializer(ValueDecl *Member,</div>
<div> assert((DirectMember || IndirectMember) &&</div><div> "Member must be a FieldDecl or IndirectFieldDecl");</div>
<div> </div><div>+ // FIXME: Allow more initializers not to count as usage, e.g. where all</div><div>+ // arguments are primitive types.</div><div>+ ValueDecl *VD = dyn_cast<ValueDecl>(Member);</div><div><br></div>
<div>If you know the cast will always succeed, just use "cast" rather than "dyn_cast".</div></div></blockquote><div><br></div></div><div>Done.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div>+ if (!VD->getType()->isBuiltinType() &&</div><div>+ !VD->getType()->isAnyPointerType() &&</div>
</div><div><br></div><div>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.</div></blockquote><div><br></div>
</div><div>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.</div><div class="im"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div>--- a/lib/Sema/SemaTemplateInstantiate.cpp</div>
<div>+++ b/lib/Sema/SemaTemplateInstantiate.cpp</div><div>@@ -1791,6 +1791,19 @@ Sema::InstantiateClass(SourceLocation PointOfInstantiation,</div><div> if (OldField->getInClassInitializer())</div><div> FieldsWithMemberInitializers.push_back(std::make_pair(OldField,</div>
<div> Field));</div><div>+ if (Field->getAccess() == AS_private) {</div><div>+ bool hasNoSideEffects = true;</div><div>+ if (!Field->getType().isNull()) {</div>
<div>+ if (const CXXRecordDecl *RD =</div><div>+ Field->getType().getTypePtrOrNull()->getAsCXXRecordDecl()) {</div><div>+ hasNoSideEffects = RD->isCompleteDefinition() &&</div>
<div>+ RD->hasTrivialDefaultConstructor() &&</div><div>+ RD->hasTrivialDestructor();</div><div>+ }</div><div>+ }</div><div>
+ if (hasNoSideEffects)</div><div>+ UnusedPrivateFields.insert(Field);</div><div>+ }</div></div><div><br></div><div>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.</div>
</blockquote></div></div><br><div>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)?</div>
</blockquote></div><br>