[patch] Implementing -Wunused-local-typedefs
Richard Smith
richard at metafoo.co.uk
Fri Sep 5 14:44:30 PDT 2014
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?)
+ // Warnigns emitted in ActOnEndOfTranslationUnit() should be emitted
for
Typo 'Warnigns'.
+ // 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 :)
+ 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.
+ if (auto* TD = dyn_cast<TypedefNameDecl>(D)) {
+ // typedefs can be referenced later on, so the diagnostics are emitted
+ // at end-of-translation-unit.
Likewise.
+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?
+/// In a function like
+/// auto f() {
+/// struct S { typedef int a; };
+/// return S;
+/// }
Should be 'return S();' or similar here.
+/// others. Pretend that all local typedefs are always references, to not
warn
Typo 'references'.
+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.
+ if (T->getAccess() != AS_private)
You should also check if the class has any friends; if so, they might use
the member.
+ }
else if (TypeDecl *TD = dyn_cast<TypeDecl>(FoundDecl))
No newline between } and else.
+ // 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)`
+ // FIXME: typedef that's only used in a constexpr
Do you mean "in a constant expression"? constexpr is an adjective, not a
noun.
On Thu, Sep 4, 2014 at 11:22 AM, Nico Weber <thakis at chromium.org> wrote:
> 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/20140905/2540bd4a/attachment.html>
More information about the cfe-commits
mailing list