Ping :-)
On Sat, Aug 30, 2014 at 3:27 PM, Nico Weber <[email protected]> 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 <[email protected]> wrote: > >> On Wed, Aug 6, 2014 at 7:05 PM, Richard Smith <[email protected]> >> 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 <[email protected]> wrote: >>> >>>> ping >>>> >>>> >>>> On Sun, Jul 27, 2014 at 8:36 PM, Nico Weber <[email protected]> >>>> 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 <[email protected]> >>>>> 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 <[email protected]> >>>>>> 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 <[email protected] >>>>>>> > 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 <[email protected]> >>>>>>>> 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 <[email protected]> >>>>>>>>> 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 <[email protected]> >>>>>>>>> wrote: >>>>>>>>> >> (Forgot to say: This is also PR18265.) >>>>>>>>> >> >>>>>>>>> >> On Sat, May 3, 2014 at 6:07 PM, Nico Weber <[email protected]> >>>>>>>>> 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. >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
