<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Sep 5, 2014 at 4:44 PM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="">On Fri, Sep 5, 2014 at 2:44 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>Thanks, this is looking really good.</div><div><br></div><div>+ if (Diags.isIgnored(diag::warn_unused_local_typedef, SourceLocation()))</div><div>+ return;</div><div><br></div><div>It would seem better to consider the diagnostic state at the point where the typedef is declared. (Maybe check this when adding typedefs to the set rather than when diagnosing?)</div></div></blockquote><div><br></div></span><div>Great catch. -Wunused-private-fields probably gets this wrong too. I added a test for this (see the very bottom of warn-unused-local-typedef.cpp, and the pragma diagnostic push/pop). The test sadly showed that adding when adding the typedefs is wrong too: For template instantiations, the instantiation is done at end-of-file, but a pragma diag at end of file shouldn't disable warnings in templates that physically are further up. So I just removed this check.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>+ // Warnigns emitted in ActOnEndOfTranslationUnit() should be emitted for<br></div><div><br></div><div>Typo 'Warnigns'.</div></div></blockquote><div><br></div></span><div>Doen.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>+ // Except for labels, we only care about unused decls that are local to<br></div><div><div>+ // functions.</div><div>+ bool WithinFunction = D->getDeclContext()->isFunctionOrMethod();</div><div>+ if (const auto *R = dyn_cast<CXXRecordDecl>(D->getDeclContext()))</div><div>+ WithinFunction =</div><div>+ WithinFunction || (R->isLocalClass() && !R->isDependentType());</div></div><div><br></div><div>Why are dependent local classes skipped here? I'd like at least a comment to explain this so future readers aren't surprised :)</div></div></blockquote><div><br></div></span><div>I added a short comment. This is covered by my tests, so removing it does make something fail. (SemaTemplateInstantiateDecl.cpp's Sema::BuildVariableInstantiation() calls this method for instantiated fields, so the warnings are deferred until after instantiation.)</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><br></div><div><div>+ for (auto *TmpD : D->decls()) {</div><div>+ if (const auto* T = dyn_cast<TypedefNameDecl>(TmpD))</div><div>+ DiagnoseUnusedDecl(T);</div><div>+ else if(const auto *R = dyn_cast<RecordDecl>(TmpD))</div></div><div><br></div><div>'*' on the right, please.</div></div></blockquote><div><br></div></span><div> Done.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><br></div><div><div>+ if (auto* TD = dyn_cast<TypedefNameDecl>(D)) {<br></div><div>+ // typedefs can be referenced later on, so the diagnostics are emitted</div><div>+ // at end-of-translation-unit.</div></div><div><br></div><div>Likewise.</div></div></blockquote><div><br></div></span><div> Done.<br></div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><br></div><div><div>+void Sema::DiagnoseUnusedNestedTypedefs(const RecordDecl *D) {</div><div>+ if (D->getTypeForDecl()->isDependentType())</div><div>+ return;</div><div>+</div><div>+ for (auto *TmpD : D->decls()) {</div><div>+ if (const auto* T = dyn_cast<TypedefNameDecl>(TmpD))</div><div>+ DiagnoseUnusedDecl(T);</div><div>+ else if(const auto *R = dyn_cast<RecordDecl>(TmpD))</div><div>+ DiagnoseUnusedNestedTypedefs(R);</div><div>+ }</div><div>+}</div></div><div>[...]</div><div><div>- if (!S->hasUnrecoverableErrorOccurred())</div><div>+ if (!S->hasUnrecoverableErrorOccurred()) {</div><div> DiagnoseUnusedDecl(D);</div><div>+ if (const auto *RD = dyn_cast<RecordDecl>(D))</div><div>+ DiagnoseUnusedNestedTypedefs(RD);</div><div>+ }</div></div><div><br></div><div>Would it make sense for DiagnoseUnusedDecl to do the recursive walk over child decls itself?</div></div></blockquote><div><br></div></span><div>Doesn't matter too much either way I guess? Left it as is for now.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><br></div><div><div>+/// In a function like</div><div>+/// auto f() {</div><div>+/// struct S { typedef int a; };</div><div>+/// return S;</div><div>+/// }</div></div><div><br></div><div>Should be 'return S();' or similar here.</div></div></blockquote><div><br></div></span><div>Done.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>+/// others. Pretend that all local typedefs are always references, to not warn<br></div><div><br></div><div>Typo 'references'.</div></div></blockquote><div><br></div></span><div>Done.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>+class LocalTypedefNameReferencer<br></div><div><div>+ : public RecursiveASTVisitor<LocalTypedefNameReferencer> {</div><div>+public:</div><div>+ LocalTypedefNameReferencer(Sema &S) : S(S) {}</div><div>+ bool VisitRecordType(const RecordType *RT);</div><div>+private:</div><div>+ Sema &S;</div><div>+};</div></div><div><br></div><div>I think you'll also need to handle MemberPointerType here; the `Class` in `T Class::*` is just modeled as a CXXRecordDecl, not a RecordType.</div></div></blockquote><div><br></div></span><div>I added the (slightly contrived :-) ) test you showed me, it seems to pass as-is.</div></div></div></div></blockquote><div><br></div><div>Does it still not warn if you remove the g() function and its use?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>+ if (T->getAccess() != AS_private)<br></div><div><br></div><div>You should also check if the class has any friends; if so, they might use the member.</div></div></blockquote><div><br></div></span><div>Good point. Done, added a test for this.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>+ }<br></div><div><div> else if (TypeDecl *TD = dyn_cast<TypeDecl>(FoundDecl))</div></div><div><br></div><div>No newline between } and else.</div></div></blockquote><div><br></div></span><div>Done.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>+ // Build a record containing all of the UnusedLocalTypedefNameCandidates.<br></div><div><div>+ RecordData UnusedLocalTypedefNameCandidates;</div><div>+ for (const TypedefNameDecl *TD : SemaRef.UnusedLocalTypedefNameCandidates)</div><div>+ AddDeclRef(TD, UnusedLocalTypedefNameCandidates);</div></div><div><br></div><div>Maybe wrap this in a `if (!isModule)`</div></div></blockquote><div><br></div></span><div>UnusedLocalTypedefNameCandidates should've been clear()ed for modules at this point I think, so should be fine as is.<br></div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>+ // FIXME: typedef that's only used in a constexpr<br></div><div><br></div><div>Do you mean "in a constant expression"? constexpr is an adjective, not a noun.</div></div></blockquote><div><br></div></span><div>Done (by fixing the FIXME).</div><div><br></div><div>Thanks for the thorough review!</div></div></div></div>
</blockquote></div><br></div></div>