<div dir="ltr">Ping :-)</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Sat, Aug 30, 2014 at 3:27 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">The next chapter in the saga, with these new features:<div>* Test for serialization to a gch file</div><div>* Code change to emit this when building a module, not when the module is used<br></div><div>* Test for modules</div><div><br>Good enough to land?</div></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><br><div class="gmail_quote">On Sat, Aug 9, 2014 at 5:46 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"><div class="gmail_extra"><div class="gmail_quote"><span>On Wed, Aug 6, 2014 at 7:05 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</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"><div>+ /// \brief Set containing all declared private fields that are not used.</div><div> typedef llvm::SmallSetVector<const NamedDecl*, 16> NamedDeclSetType;</div>
<div>-</div><div>- /// \brief Set containing all declared private fields that are not used.</div>
<div> NamedDeclSetType UnusedPrivateFields;</div><div><br></div><div>Doesn't this make Doxygen attach the documentation to the wrong thing?</div></div></blockquote><div><br></div></span><div>Fixed.</div><span><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div>+ /// \brief Set containing all typedefs that are likely unused.<br></div><div>
<div>+ llvm::SmallSetVector<const TypedefNameDecl *, 4></div><div>+ UnusedLocalTypedefNameCandidates;</div></div><div><br></div><div>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).</div>
</div></blockquote><div><br></div></span><div>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).</div><div><br></div>
<div>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.)</div><span>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">
<div><br></div><div>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.</div>
</div></blockquote><div><br></div></span><div>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.</div><span>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">
<div>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.<br>
</div>
<div><br></div><div>Presumably we can apply the unused-local-class-member-typedef diagnostic mechanism to cover *all* local class members?</div></div></blockquote><div><br></div></span><div>That's a good idea for a follow-up, yes.</div><div><div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Aug 6, 2014 at 6:08 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">ping</div><div><div><div class="gmail_extra"><br><br><div class="gmail_quote">
On Sun, Jul 27, 2014 at 8:36 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">This now also warns on<div><br></div><div><div><div>template <class T></div><div>void template_fun(T t) {</div>
</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><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><div><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>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>