<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Sep 5, 2014 at 6:37 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><div class="h5"><div class="gmail_extra"><div class="gmail_quote">On Fri, Sep 5, 2014 at 6:01 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 class="gmail_extra"><div class="gmail_quote"><div><div>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: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="gmail_extra"><div class="gmail_quote"><span>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><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><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><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><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><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><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><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><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></div><div>Does it still not warn if you remove the g() function and its use?</div></div></div></div></blockquote></div><br></div></div></div><div class="gmail_extra">Yes, as discussed on IRC, RecursiveASTVisitor gets this right for me.</div><div class="gmail_extra"><br></div><div class="gmail_extra">As also discussed on IRC, landed in r217298.</div></div></blockquote><div><br></div><div>(For posterity: LGTM)</div><div> </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">Thanks for the great review, and for realizing that this warning is trickier to get right than I (and the person who implemented it in gcc ;-) ) thought – I learned quite a bit about clang while working on this :-)</div></div>
</blockquote></div><br></div></div>