[patch] Implementing -Wunused-local-typedefs
Nico Weber
thakis at chromium.org
Thu Sep 4 11:22:55 PDT 2014
Ping :-)
On Sat, Aug 30, 2014 at 3:27 PM, Nico Weber <thakis at chromium.org> wrote:
> The next chapter in the saga, with these new features:
> * Test for serialization to a gch file
> * Code change to emit this when building a module, not when the module is
> used
> * Test for modules
>
> Good enough to land?
>
>
> On Sat, Aug 9, 2014 at 5:46 PM, Nico Weber <thakis at chromium.org> wrote:
>
>> On Wed, Aug 6, 2014 at 7:05 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>>
>>> + /// \brief Set containing all declared private fields that are not
>>> used.
>>> typedef llvm::SmallSetVector<const NamedDecl*, 16> NamedDeclSetType;
>>> -
>>> - /// \brief Set containing all declared private fields that are not
>>> used.
>>> NamedDeclSetType UnusedPrivateFields;
>>>
>>> Doesn't this make Doxygen attach the documentation to the wrong thing?
>>>
>>
>> Fixed.
>>
>>
>>> + /// \brief Set containing all typedefs that are likely unused.
>>> + llvm::SmallSetVector<const TypedefNameDecl *, 4>
>>> + UnusedLocalTypedefNameCandidates;
>>>
>>> You don't seem to have any serialization/deserialization code for this;
>>> I don't think this is doing quite the right thing in all the AST-file
>>> cases. Preambles and PCH should behave exactly like we'd actually included
>>> the file, so in those cases you'd need to serialize/deserialize. For
>>> modules, you probably want to issue the diagnostic while building the
>>> module (you currently won't, because the emission code is after the end of
>>> the module).
>>>
>>
>> Added serialization code. For modules, the diagnostics are emitted when
>> the module is used, not when it's build (this seems to match the other
>> unused warnings).
>>
>> Are there good example for how to test the serialization bits? -verify
>> doesn't work well with .ast files as far as I can tell. (I guess I could
>> have a similar test that uses FileCheck instead of -verify, but that's kind
>> of lame.)
>>
>>
>>>
>>> I've thought about the C++14 deduced return type issue a bit more, and I
>>> think there's a better way of handling it: when we deduce a return type for
>>> a function, if the type involves any local class types, mark all of their
>>> members as referenced. (Those members have potentially escaped our
>>> analysis, even if they're not used in this translation unit.) We could be
>>> smarter here and do different things if the function is not an external
>>> linkage inline function (delay until end of TU, perhaps), but I don't think
>>> it's an important case. The current warning would have false positives if
>>> an escaped type's typedefs are used in one TU but not another.
>>>
>>
>> Done, including the internal linkage thing you suggested. I also don't
>> mark private typedefs as referenced. I kept the "delay everything until the
>> end" for now, since it's needed in the general case, and having just one
>> codepath that emits this warning seems nice.
>>
>>
>>> One other thing: we only need to delay diagnosing unused local typedefs
>>> if they're local class members, I think -- otherwise, we should see the use
>>> before they get popped. Even then, the only way we can get a delayed use is
>>> if they're involved in a template argument in some way. We could possibly
>>> detect that case and treat them as escaped, but perhaps that would be too
>>> expensive.
>>>
>>> Presumably we can apply the unused-local-class-member-typedef diagnostic
>>> mechanism to cover *all* local class members?
>>>
>>
>> That's a good idea for a follow-up, yes.
>>
>>
>>>
>>>
>>> On Wed, Aug 6, 2014 at 6:08 PM, Nico Weber <thakis at chromium.org> wrote:
>>>
>>>> ping
>>>>
>>>>
>>>> On Sun, Jul 27, 2014 at 8:36 PM, Nico Weber <thakis at chromium.org>
>>>> wrote:
>>>>
>>>>> This now also warns on
>>>>>
>>>>> template <class T>
>>>>> void template_fun(T t) {
>>>>> struct S2 {
>>>>> typedef int Foo1;
>>>>> };
>>>>> }
>>>>> void template_fun_user() {
>>>>> template_fun(3);
>>>>> }
>>>>>
>>>>> That is, it fixes the FIXME mentioned in the last post (by checking
>>>>> for unused typedefs in TemplateDeclInstantiator::VisitCXXRecordDecl after
>>>>> the record decl has been instantiated). I also
>>>>> changed ShouldDiagnoseUnusedDecl() to not take a new parameter but instead
>>>>> compute WithinFunction locally like you suggested.
>>>>>
>>>>> (I also removed a redundant IsUsed() check
>>>>> in Sema::BuildVariableInstantiation() – DiagnoseUnusedDecl() checks that
>>>>> already.)
>>>>>
>>>>> I'll stop futzing with this now; I think this is read for a look. (Not
>>>>> urgent at all, of course.)
>>>>>
>>>>>
>>>>> On Sun, Jul 27, 2014 at 5:13 PM, Nico Weber <thakis at chromium.org>
>>>>> wrote:
>>>>>
>>>>>> Now gets this right:
>>>>>>
>>>>>> template <class T>
>>>>>> void template_fun(T t) {
>>>>>> typename T::Foo s3foo;
>>>>>> }
>>>>>> void template_fun_user() {
>>>>>> struct Local {
>>>>>> typedef int Foo; // no-diag
>>>>>> typedef int Bar; // expected-warning {{unused typedef 'Bar'}}
>>>>>> } p;
>>>>>> template_fun(p);
>>>>>> }
>>>>>>
>>>>>> It also no longer warns on S::Foo in:
>>>>>>
>>>>>> template<class T>
>>>>>> void tf(T t) {
>>>>>> struct S {
>>>>>> typedef int Foo;
>>>>>> typedef int Bar;
>>>>>> };
>>>>>> S::Foo f;
>>>>>> }
>>>>>>
>>>>>> (It stopped warning on S::Bar too – there's a FIXME in the test that
>>>>>> explains why and how to fix that.)
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Sat, Jul 26, 2014 at 9:26 PM, Nico Weber <thakis at chromium.org>
>>>>>> wrote:
>>>>>>
>>>>>>> I'm delighted to announce that, after almost two months of hard
>>>>>>> work, a new patch set is available for review!
>>>>>>>
>>>>>>> With these tantalizing features and fixes:
>>>>>>> * TypedefNameDecls everywhere
>>>>>>> * MarkAnyDeclReferenced() used throughout
>>>>>>> * Instead of immediately emitting a diag, the TypedefNameDecls are
>>>>>>> stashed in a set and the diagnostics for things in the set are emitted at
>>>>>>> end-of-TU (except for TypedefNameDecls that have been referenced since then)
>>>>>>> * It's now called -Wunused-local-typedef in singular, with an alias
>>>>>>> for the gcc spelling
>>>>>>> * More tests
>>>>>>>
>>>>>>> On Tue, May 6, 2014 at 6:01 PM, Richard Smith <richard at metafoo.co.uk
>>>>>>> > wrote:
>>>>>>>
>>>>>>>> def UnusedConstVariable : DiagGroup<"unused-const-variable">;
>>>>>>>> def UnusedVariable : DiagGroup<"unused-variable",
>>>>>>>> [UnusedConstVariable]>;
>>>>>>>> +def UnusedLocalTypedefs : DiagGroup<"unused-local-typedefs">;
>>>>>>>> def UnusedPropertyIvar : DiagGroup<"unused-property-ivar">;
>>>>>>>>
>>>>>>>> I'm a little uncomfortable about this flag being plural and the
>>>>>>>> others being singular. GCC compatible, you say? =(
>>>>>>>>
>>>>>>>>
>>>>>>>> +def warn_unused_local_typedefs : Warning<"unused typedef %0">,
>>>>>>>>
>>>>>>>> This one should definitely be singular.
>>>>>>>>
>>>>>>>> Reformatting of lib/Parse/ParseExprCXX.cpp looks unrelated. Feel
>>>>>>>> free to commit that separately.
>>>>>>>>
>>>>>>>> + if (TypedefDecl* TD = dyn_cast_or_null<TypedefDecl>(SD))
>>>>>>>>
>>>>>>>> * should go on the right. Also, use 'auto' for a variable
>>>>>>>> initialized from cast/dyn_cast/etc.
>>>>>>>>
>>>>>>>> Rather than calling setReferenced directly, please call
>>>>>>>> Sema::MarkAnyDeclReferenced.
>>>>>>>>
>>>>>>>> +static bool ShouldDiagnoseUnusedDecl(const NamedDecl *D,
>>>>>>>> + const DeclContext *DC) {
>>>>>>>> [...]
>>>>>>>> + if (!DC->isFunctionOrMethod())
>>>>>>>> + return false;
>>>>>>>>
>>>>>>>> Please document what 'DC' is here; it's really not obvious. But it
>>>>>>>> seems to me that you can replace DC here and below with just a bool
>>>>>>>> WithinFunction.
>>>>>>>>
>>>>>>>
>>>>>>> Done.
>>>>>>>
>>>>>>>
>>>>>>>> You can also remove it entirely and check whether
>>>>>>>> D->getDeclContext() is a function, method, or local class.
>>>>>>>>
>>>>>>>> +void Sema::DiagnoseUnusedDecl(const NamedDecl *D, const
>>>>>>>> DeclContext *DC) {
>>>>>>>> + if (DC->isFunctionOrMethod())
>>>>>>>> + if (const RecordDecl *RD = dyn_cast<RecordDecl>(D))
>>>>>>>> + for (auto *TmpD : RD->decls())
>>>>>>>> + if (isa<TypedefDecl>(TmpD) || isa<RecordDecl>(TmpD))
>>>>>>>> + DiagnoseUnusedDecl(cast<NamedDecl>(TmpD), DC);
>>>>>>>>
>>>>>>>> This would make more sense as a layer between ActOnPopScope and
>>>>>>>> DiagnoseUnusedDecl.
>>>>>>>>
>>>>>>>
>>>>>>> Done.
>>>>>>>
>>>>>>>
>>>>>>>> A comment explaining why we only do this within a function or
>>>>>>>> method would help ("within a function or method, we defer these checks
>>>>>>>> until the end of the surrounding scope").
>>>>>>>>
>>>>>>>> Also, this isn't necessarily correct in C++14, since a local type's
>>>>>>>> members can be first used once we've already left the function:
>>>>>>>>
>>>>>>>> auto f() {
>>>>>>>> struct S { typedef int t; };
>>>>>>>> return S();
>>>>>>>> }
>>>>>>>> auto x = f();
>>>>>>>> decltype(x)::t y;
>>>>>>>>
>>>>>>>> What happens if the local type is used as an argument to a function
>>>>>>>> template, which in turn uses the typedef? Can we really diagnose this
>>>>>>>> before end-of-TU?
>>>>>>>>
>>>>>>>
>>>>>>> Done.
>>>>>>>
>>>>>>>
>>>>>>>> + else if (isa<TypedefDecl>(D)) // TODO: Warn on unused c++11
>>>>>>>> aliases too?
>>>>>>>> + DiagID = diag::warn_unused_local_typedefs;
>>>>>>>>
>>>>>>>> Yes, please change this to TypedefNameDecl (throughout the patch).
>>>>>>>> It doesn't make sense to handle these differently, since one can be a
>>>>>>>> redeclaration of the other.
>>>>>>>>
>>>>>>>
>>>>>>> Done.
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
>>>>>>>> ===================================================================
>>>>>>>> --- lib/Sema/SemaTemplateInstantiateDecl.cpp (revision 207920)
>>>>>>>> +++ lib/Sema/SemaTemplateInstantiateDecl.cpp (working copy)
>>>>>>>> @@ -3651,7 +3651,7 @@
>>>>>>>> if (!NewVar->isInvalidDecl() &&
>>>>>>>> NewVar->getDeclContext()->isFunctionOrMethod() &&
>>>>>>>> !NewVar->isUsed() &&
>>>>>>>> OldVar->getType()->isDependentType())
>>>>>>>> - DiagnoseUnusedDecl(NewVar);
>>>>>>>> + DiagnoseUnusedDecl(NewVar, NewVar->getDeclContext());
>>>>>>>>
>>>>>>>> Hmm, why do we diagnose unused decls from template instantiation at
>>>>>>>> all?
>>>>>>>>
>>>>>>>
>>>>>>> r103362 added this, r133580 tweaked it some. Can you think of an
>>>>>>> example where a template-local typedef is unused only in some
>>>>>>> instantiations of that template?
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> On Sat, May 3, 2014 at 10:37 PM, Nico Weber <thakis at chromium.org>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> With one more setReferenced() call (in SemaCXXScopeSpec.cpp). This
>>>>>>>>> version has successfully compiled several thousand files of
>>>>>>>>> chromium
>>>>>>>>> (all that I've tried so far).
>>>>>>>>>
>>>>>>>>> On Sat, May 3, 2014 at 6:35 PM, Nico Weber <thakis at chromium.org>
>>>>>>>>> wrote:
>>>>>>>>> > About point 2: The patch currently incorrectly warns on the "v"
>>>>>>>>> typedef in:
>>>>>>>>> >
>>>>>>>>> > template <class T> struct V { typedef int iterator; };
>>>>>>>>> > void f() {
>>>>>>>>> > typedef V<int> v;
>>>>>>>>> > v::iterator it;
>>>>>>>>> > }
>>>>>>>>> >
>>>>>>>>> > So there's at least one more place where I need to insert a
>>>>>>>>> > setReferenced() I'll send an updated version once I found where,
>>>>>>>>> but
>>>>>>>>> > I'm thankful for comments on the overall approach in the
>>>>>>>>> meantime too.
>>>>>>>>> >
>>>>>>>>> > On Sat, May 3, 2014 at 6:08 PM, Nico Weber <thakis at chromium.org>
>>>>>>>>> wrote:
>>>>>>>>> >> (Forgot to say: This is also PR18265.)
>>>>>>>>> >>
>>>>>>>>> >> On Sat, May 3, 2014 at 6:07 PM, Nico Weber <thakis at chromium.org>
>>>>>>>>> wrote:
>>>>>>>>> >>> Hi,
>>>>>>>>> >>>
>>>>>>>>> >>> gcc has a warning -Wunused-local-typedefs that warns on unused
>>>>>>>>> local
>>>>>>>>> >>> typedefs. Inspired by r207870, I thought it'd be nice if clang
>>>>>>>>> had
>>>>>>>>> >>> this warning too. This warning requires the following three
>>>>>>>>> things:
>>>>>>>>> >>>
>>>>>>>>> >>>
>>>>>>>>> >>> 1.) A bit to track if a typedef has been referenced.
>>>>>>>>> >>>
>>>>>>>>> >>> Decl already has isUsed and isReferenced, but they don't seem
>>>>>>>>> to be
>>>>>>>>> >>> used for TypedefDecls, so I'm just using isReferenced on
>>>>>>>>> TypedefDecls
>>>>>>>>> >>> for this.
>>>>>>>>> >>>
>>>>>>>>> >>>
>>>>>>>>> >>> 2.) Setting that bit on typedefs that are used.
>>>>>>>>> >>>
>>>>>>>>> >>> The three strategies I can think of:
>>>>>>>>> >>> a.) Do this in Sema::DiagnoseUseOfDecl(), this seems to be
>>>>>>>>> already
>>>>>>>>> >>> called for decls all over the place, and an "if
>>>>>>>>> isa<TypedefDecl>(D)
>>>>>>>>> >>> D->setReferenced()" seems to do the trick.
>>>>>>>>> >>> b.) Do this in LookupName
>>>>>>>>> >>> c.) Do this explicitly in the places where it's actually
>>>>>>>>> necessary.
>>>>>>>>> >>>
>>>>>>>>> >>> The attached patch goes with the last approach. The three
>>>>>>>>> places I
>>>>>>>>> >>> found where it's needed are Sema::getTypeName(),
>>>>>>>>> Sema::ClassifyName(),
>>>>>>>>> >>> and Sema::LookupInlineAsmField(). The first two are called by
>>>>>>>>> the
>>>>>>>>> >>> parser for almost everything, while the third is called for
>>>>>>>>> references
>>>>>>>>> >>> to structs from MS inline assembly. That last one is a bit
>>>>>>>>> scary as it
>>>>>>>>> >>> means it's possible that there are other places this is needed
>>>>>>>>> that I
>>>>>>>>> >>> missed.
>>>>>>>>> >>>
>>>>>>>>> >>>
>>>>>>>>> >>> 3.) A way to check all typedefs in a scope when that scope
>>>>>>>>> ends.
>>>>>>>>> >>>
>>>>>>>>> >>> I added this to Sema::ActOnPopScope() which is where the
>>>>>>>>> >>> unused-variable and unused-label warnings are created. This
>>>>>>>>> works
>>>>>>>>> >>> well, but to catch the unused local typedef in
>>>>>>>>> >>>
>>>>>>>>> >>> {
>>>>>>>>> >>> struct A {
>>>>>>>>> >>> struct B { typedef int SoDeep; };
>>>>>>>>> >>> };
>>>>>>>>> >>> }
>>>>>>>>> >>>
>>>>>>>>> >>> it means that that code now needs to recurse into all
>>>>>>>>> RecordDecl in a
>>>>>>>>> >>> scope, which it didn't need to do previously.
>>>>>>>>> >>>
>>>>>>>>> >>>
>>>>>>>>> >>> Let me know how badly I've chosen in each instance :-)
>>>>>>>>> >>>
>>>>>>>>> >>> Thanks,
>>>>>>>>> >>> Nico
>>>>>>>>> >>>
>>>>>>>>> >>> ps: If this goes in, a follow-up question is if this should
>>>>>>>>> warn on
>>>>>>>>> >>> unused C++11 type aliases too – it'd just require
>>>>>>>>> >>> s/TypedefDecl/TypedefNameDecl/ in two places in the patch, so
>>>>>>>>> it
>>>>>>>>> >>> should be an easy follow-up.
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140904/f07ca212/attachment.html>
More information about the cfe-commits
mailing list