[patch] Implementing -Wunused-local-typedefs

[Richard Smith]

On Fri, Sep 5, 2014 at 4:44 PM, Nico Weber <thakis at chromium.org> wrote:

> On Fri, Sep 5, 2014 at 2:44 PM, Richard Smith <richard at metafoo.co.uk>
> 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!
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140905/4d108a15/attachment.html>