[patch] Implementing -Wunused-local-typedefs
Richard Smith
richard at metafoo.co.uk
Wed Aug 6 19:05:51 PDT 2014
+ /// \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?
+ /// \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).
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.
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?
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/20140806/2e0f7306/attachment.html>
More information about the cfe-commits
mailing list