[patch] Implementing -Wunused-local-typedefs

[Nico Weber]

On Fri, Sep 5, 2014 at 6:01 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> 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?
>

Yes, as discussed on IRC, RecursiveASTVisitor gets this right for me.

As also discussed on IRC, landed in r217298. 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 :-)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140905/fdf9a563/attachment.html>