<div dir="ltr">This now also warns on<div><br></div><div><div>template <class T></div><div>void template_fun(T t) {</div><div> struct S2 {</div><div> typedef int Foo1;</div><div> };</div><div>}</div><div>void template_fun_user() {</div>
<div> template_fun(3);</div><div>}</div></div><div><br></div><div>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.</div>
<div><br></div><div>(I also removed a redundant IsUsed() check in Sema::BuildVariableInstantiation() – DiagnoseUnusedDecl() checks that already.)</div><div><br></div><div>I'll stop futzing with this now; I think this is read for a look. (Not urgent at all, of course.)</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Sun, Jul 27, 2014 at 5:13 PM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Now gets this right:<div><br></div><div><div>template <class T></div><div>void template_fun(T t) {</div>
<div> typename T::Foo s3foo;<br></div><div>}</div><div>void template_fun_user() {</div><div> struct Local {</div>
<div> typedef int Foo; // no-diag</div><div> typedef int Bar; // expected-warning {{unused typedef 'Bar'}}</div><div> } p;</div><div> template_fun(p);</div><div>}</div></div><div><br></div><div>It also no longer warns on S::Foo in:</div>
<div><br></div><div>template<class T></div><div>void tf(T t) {</div><div> struct S {</div><div> typedef int Foo;</div><div> typedef int Bar;</div><div> };</div><div> S::Foo f;</div><div>}</div><div><br></div>
<div>(It stopped warning on S::Bar too – there's a FIXME in the test that explains why and how to fix that.)</div><div><br></div></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><br><div class="gmail_quote">
On Sat, Jul 26, 2014 at 9:26 PM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">I'm delighted to announce that, after almost two months of hard work, a new patch set is available for review!<div>
<br></div><div>With these tantalizing features and fixes:</div><div>* TypedefNameDecls everywhere</div>
<div>* MarkAnyDeclReferenced() used throughout</div><div>* 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)</div>
<div>* It's now called -Wunused-local-typedef in singular, with an alias for the gcc spelling</div><div>* More tests</div><div class="gmail_extra"><br><div class="gmail_quote"><div>On Tue, May 6, 2014 at 6:01 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
</div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><div> def UnusedConstVariable : DiagGroup<"unused-const-variable">;</div>
<div> def UnusedVariable : DiagGroup<"unused-variable",</div><div> [UnusedConstVariable]>;</div>
<div>+def UnusedLocalTypedefs : DiagGroup<"unused-local-typedefs">;</div><div> def UnusedPropertyIvar : DiagGroup<"unused-property-ivar">;</div><div class="gmail_extra"><br></div></div><div class="gmail_extra">
I'm a little uncomfortable about this flag being plural and the others being singular. GCC compatible, you say? =(</div><div><div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">
+def warn_unused_local_typedefs : Warning<"unused typedef %0">,</div>
<div class="gmail_extra"><br></div></div><div class="gmail_extra">This one should definitely be singular.</div><div class="gmail_extra"><br></div><div class="gmail_extra">Reformatting of lib/Parse/ParseExprCXX.cpp looks unrelated. Feel free to commit that separately.<br>
</div><div><div class="gmail_extra"><br></div><div class="gmail_extra">+ if (TypedefDecl* TD = dyn_cast_or_null<TypedefDecl>(SD))<br></div><div class="gmail_extra"><br></div></div><div class="gmail_extra">
* should go on the right. Also, use 'auto' for a variable initialized from cast/dyn_cast/etc.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">Rather than calling setReferenced directly, please call Sema::MarkAnyDeclReferenced.</div><div class="gmail_extra"><div><div class="gmail_extra"><br></div>
<div class="gmail_extra">
+static bool ShouldDiagnoseUnusedDecl(const NamedDecl *D,</div><div class="gmail_extra">+ const DeclContext *DC) {</div></div><div class="gmail_extra">[...]</div></div><div class="gmail_extra">
<div>
<div class="gmail_extra">+ if (!DC->isFunctionOrMethod())</div><div class="gmail_extra">+ return false;</div><div><br></div></div><div>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.</div>
</div></div></blockquote><div><br></div></div><div>Done.</div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div> You can also remove it entirely and check whether D->getDeclContext() is a function, method, or local class.<br>
</div></div><div class="gmail_extra"><br></div><div class="gmail_extra"><div><div class="gmail_extra">
+void Sema::DiagnoseUnusedDecl(const NamedDecl *D, const DeclContext *DC) {</div></div><div><div class="gmail_extra">+ if (DC->isFunctionOrMethod())</div><div class="gmail_extra"><div class="gmail_extra">+ if (const RecordDecl *RD = dyn_cast<RecordDecl>(D))</div>
<div class="gmail_extra">+ for (auto *TmpD : RD->decls())</div><div class="gmail_extra">+ if (isa<TypedefDecl>(TmpD) || isa<RecordDecl>(TmpD))</div><div class="gmail_extra">+ DiagnoseUnusedDecl(cast<NamedDecl>(TmpD), DC);</div>
</div><div class="gmail_extra"><br></div></div><div class="gmail_extra">This would make more sense as a layer between ActOnPopScope and DiagnoseUnusedDecl.</div></div></div></blockquote><div><br></div></div><div>Done.</div>
<div><div>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_extra">
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").</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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:</div><div class="gmail_extra">
<br></div><div class="gmail_extra"> auto f() {</div><div class="gmail_extra"> struct S { typedef int t; };</div><div class="gmail_extra"> return S();</div><div class="gmail_extra"> }</div><div class="gmail_extra">
auto x = f();</div><div class="gmail_extra"> decltype(x)::t y;</div><div class="gmail_extra"><br></div><div class="gmail_extra">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?<br>
</div></div></div></blockquote><div><br></div></div><div>Done.</div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_extra">
</div>
</div><div class="gmail_extra">+ else if (isa<TypedefDecl>(D)) // TODO: Warn on unused c++11 aliases too?<br></div><div class="gmail_extra"><div><div class="gmail_extra">+ DiagID = diag::warn_unused_local_typedefs;</div>
<div><br></div></div><div>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.</div></div></div></blockquote>
<div><br></div></div><div>Done.</div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr">
<div class="gmail_extra">
<br></div><div class="gmail_extra"><div><div class="gmail_extra">Index: lib/Sema/SemaTemplateInstantiateDecl.cpp</div><div class="gmail_extra">===================================================================</div>
<div class="gmail_extra">
--- lib/Sema/SemaTemplateInstantiateDecl.cpp<span style="white-space:pre-wrap"> </span>(revision 207920)</div><div class="gmail_extra">+++ lib/Sema/SemaTemplateInstantiateDecl.cpp<span style="white-space:pre-wrap"> </span>(working copy)</div>
<div class="gmail_extra">@@ -3651,7 +3651,7 @@</div><div class="gmail_extra"> if (!NewVar->isInvalidDecl() &&</div><div class="gmail_extra"> NewVar->getDeclContext()->isFunctionOrMethod() && !NewVar->isUsed() &&</div>
<div class="gmail_extra"> OldVar->getType()->isDependentType())</div><div class="gmail_extra">- DiagnoseUnusedDecl(NewVar);</div><div class="gmail_extra">+ DiagnoseUnusedDecl(NewVar, NewVar->getDeclContext());</div>
<div class="gmail_extra"><br></div></div><div class="gmail_extra">Hmm, why do we diagnose unused decls from template instantiation at all?</div></div></div></blockquote><div><br></div></div><div>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?<br>
</div><div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>
<div><div class="gmail_extra">
<br><div class="gmail_quote">On Sat, May 3, 2014 at 10:37 PM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">With one more setReferenced() call (in SemaCXXScopeSpec.cpp). This<br>
version has successfully compiled several thousand files of chromium<br>
(all that I've tried so far).<br>
<div><div><br>
On Sat, May 3, 2014 at 6:35 PM, Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>> wrote:<br>
> About point 2: The patch currently incorrectly warns on the "v" typedef in:<br>
><br>
> template <class T> struct V { typedef int iterator; };<br>
> void f() {<br>
> typedef V<int> v;<br>
> v::iterator it;<br>
> }<br>
><br>
> So there's at least one more place where I need to insert a<br>
> setReferenced() I'll send an updated version once I found where, but<br>
> I'm thankful for comments on the overall approach in the meantime too.<br>
><br>
> On Sat, May 3, 2014 at 6:08 PM, Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>> wrote:<br>
>> (Forgot to say: This is also PR18265.)<br>
>><br>
>> On Sat, May 3, 2014 at 6:07 PM, Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>> wrote:<br>
>>> Hi,<br>
>>><br>
>>> gcc has a warning -Wunused-local-typedefs that warns on unused local<br>
>>> typedefs. Inspired by r207870, I thought it'd be nice if clang had<br>
>>> this warning too. This warning requires the following three things:<br>
>>><br>
>>><br>
>>> 1.) A bit to track if a typedef has been referenced.<br>
>>><br>
>>> Decl already has isUsed and isReferenced, but they don't seem to be<br>
>>> used for TypedefDecls, so I'm just using isReferenced on TypedefDecls<br>
>>> for this.<br>
>>><br>
>>><br>
>>> 2.) Setting that bit on typedefs that are used.<br>
>>><br>
>>> The three strategies I can think of:<br>
>>> a.) Do this in Sema::DiagnoseUseOfDecl(), this seems to be already<br>
>>> called for decls all over the place, and an "if isa<TypedefDecl>(D)<br>
>>> D->setReferenced()" seems to do the trick.<br>
>>> b.) Do this in LookupName<br>
>>> c.) Do this explicitly in the places where it's actually necessary.<br>
>>><br>
>>> The attached patch goes with the last approach. The three places I<br>
>>> found where it's needed are Sema::getTypeName(), Sema::ClassifyName(),<br>
>>> and Sema::LookupInlineAsmField(). The first two are called by the<br>
>>> parser for almost everything, while the third is called for references<br>
>>> to structs from MS inline assembly. That last one is a bit scary as it<br>
>>> means it's possible that there are other places this is needed that I<br>
>>> missed.<br>
>>><br>
>>><br>
>>> 3.) A way to check all typedefs in a scope when that scope ends.<br>
>>><br>
>>> I added this to Sema::ActOnPopScope() which is where the<br>
>>> unused-variable and unused-label warnings are created. This works<br>
>>> well, but to catch the unused local typedef in<br>
>>><br>
>>> {<br>
>>> struct A {<br>
>>> struct B { typedef int SoDeep; };<br>
>>> };<br>
>>> }<br>
>>><br>
>>> it means that that code now needs to recurse into all RecordDecl in a<br>
>>> scope, which it didn't need to do previously.<br>
>>><br>
>>><br>
>>> Let me know how badly I've chosen in each instance :-)<br>
>>><br>
>>> Thanks,<br>
>>> Nico<br>
>>><br>
>>> ps: If this goes in, a follow-up question is if this should warn on<br>
>>> unused C++11 type aliases too – it'd just require<br>
>>> s/TypedefDecl/TypedefNameDecl/ in two places in the patch, so it<br>
>>> should be an easy follow-up.<br>
</div></div></blockquote></div><br></div></div></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>