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? + if (T->getAccess() != AS_private) >> >> You should also check if the class has any friends; if so, they might use >> the member. >> > > Good point. Done, added a test for this. > > >> + } >> else if (TypeDecl *TD = dyn_cast<TypeDecl>(FoundDecl)) >> >> No newline between } and else. >> > > Done. > > >> + // Build a record containing all of the >> UnusedLocalTypedefNameCandidates. >> + RecordData UnusedLocalTypedefNameCandidates; >> + for (const TypedefNameDecl *TD : >> SemaRef.UnusedLocalTypedefNameCandidates) >> + AddDeclRef(TD, UnusedLocalTypedefNameCandidates); >> >> Maybe wrap this in a `if (!isModule)` >> > > UnusedLocalTypedefNameCandidates should've been clear()ed for modules at > this point I think, so should be fine as is. > > >> + // FIXME: typedef that's only used in a constexpr >> >> Do you mean "in a constant expression"? constexpr is an adjective, not a >> noun. >> > > Done (by fixing the FIXME). > > Thanks for the thorough review! >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
