[patch] Implementing -Wunused-local-typedefs

[Richard Smith]

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

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

(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 :-)
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140905/602462a5/attachment.html>