On Fri, Sep 5, 2014 at 6:37 PM, Nico Weber <[email protected]> wrote:
> On Fri, Sep 5, 2014 at 6:01 PM, Richard Smith <[email protected]> > wrote: > >> On Fri, Sep 5, 2014 at 4:44 PM, Nico Weber <[email protected]> wrote: >> >>> On Fri, Sep 5, 2014 at 2:44 PM, Richard Smith <[email protected]> >>> wrote: >>> >>>> Thanks, this is looking really good. >>>> >>>> + if (Diags.isIgnored(diag::warn_unused_local_typedef, >>>> SourceLocation())) >>>> + return; >>>> >>>> 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?) >>>> >>> >>> 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. >>> >>> >>>> + // Warnigns emitted in ActOnEndOfTranslationUnit() should be >>>> emitted for >>>> >>>> Typo 'Warnigns'. >>>> >>> >>> Doen. >>> >>> >>>> + // Except for labels, we only care about unused decls that are local >>>> to >>>> + // functions. >>>> + bool WithinFunction = D->getDeclContext()->isFunctionOrMethod(); >>>> + if (const auto *R = dyn_cast<CXXRecordDecl>(D->getDeclContext())) >>>> + WithinFunction = >>>> + WithinFunction || (R->isLocalClass() && !R->isDependentType()); >>>> >>>> Why are dependent local classes skipped here? I'd like at least a >>>> comment to explain this so future readers aren't surprised :) >>>> >>> >>> 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.) >>> >>> >>>> >>>> + for (auto *TmpD : D->decls()) { >>>> + if (const auto* T = dyn_cast<TypedefNameDecl>(TmpD)) >>>> + DiagnoseUnusedDecl(T); >>>> + else if(const auto *R = dyn_cast<RecordDecl>(TmpD)) >>>> >>>> '*' on the right, please. >>>> >>> >>> >>> Done. >>> >>> >>>> >>>> + if (auto* TD = dyn_cast<TypedefNameDecl>(D)) { >>>> + // typedefs can be referenced later on, so the diagnostics are >>>> emitted >>>> + // at end-of-translation-unit. >>>> >>>> Likewise. >>>> >>> >>> >>> Done. >>> >>> >>>> >>>> +void Sema::DiagnoseUnusedNestedTypedefs(const RecordDecl *D) { >>>> + if (D->getTypeForDecl()->isDependentType()) >>>> + return; >>>> + >>>> + for (auto *TmpD : D->decls()) { >>>> + if (const auto* T = dyn_cast<TypedefNameDecl>(TmpD)) >>>> + DiagnoseUnusedDecl(T); >>>> + else if(const auto *R = dyn_cast<RecordDecl>(TmpD)) >>>> + DiagnoseUnusedNestedTypedefs(R); >>>> + } >>>> +} >>>> [...] >>>> - if (!S->hasUnrecoverableErrorOccurred()) >>>> + if (!S->hasUnrecoverableErrorOccurred()) { >>>> DiagnoseUnusedDecl(D); >>>> + if (const auto *RD = dyn_cast<RecordDecl>(D)) >>>> + DiagnoseUnusedNestedTypedefs(RD); >>>> + } >>>> >>>> Would it make sense for DiagnoseUnusedDecl to do the recursive walk >>>> over child decls itself? >>>> >>> >>> Doesn't matter too much either way I guess? Left it as is for now. >>> >>> >>>> >>>> +/// In a function like >>>> +/// auto f() { >>>> +/// struct S { typedef int a; }; >>>> +/// return S; >>>> +/// } >>>> >>>> Should be 'return S();' or similar here. >>>> >>> >>> Done. >>> >>> >>>> +/// others. Pretend that all local typedefs are always references, to >>>> not warn >>>> >>>> Typo 'references'. >>>> >>> >>> Done. >>> >>> >>>> +class LocalTypedefNameReferencer >>>> + : public RecursiveASTVisitor<LocalTypedefNameReferencer> { >>>> +public: >>>> + LocalTypedefNameReferencer(Sema &S) : S(S) {} >>>> + bool VisitRecordType(const RecordType *RT); >>>> +private: >>>> + Sema &S; >>>> +}; >>>> >>>> I think you'll also need to handle MemberPointerType here; the `Class` >>>> in `T Class::*` is just modeled as a CXXRecordDecl, not a RecordType. >>>> >>> >>> I added the (slightly contrived :-) ) test you showed me, it seems to >>> pass as-is. >>> >> >> Does it still not warn if you remove the g() function and its use? >> > > Yes, as discussed on IRC, RecursiveASTVisitor gets this right for me. > > As also discussed on IRC, landed in r217298. > (For posterity: LGTM) > 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 :-) >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
