<div dir="ltr">Sent <a href="https://reviews.llvm.org/D112765">https://reviews.llvm.org/D112765</a>.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Oct 28, 2021 at 11:45 PM Sam McCall <<a href="mailto:sammccall@google.com">sammccall@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Looks like originally injected-class-names were considered redecls, then this was changed for CXXRecordDecl in 2010: <a href="https://github.com/llvm/llvm-project/commit/470c454a6176ef31474553e408c90f5ee630df89" target="_blank">https://github.com/llvm/llvm-project/commit/470c454a6176ef31474553e408c90f5ee630df89</a> but the corresponding place where templates were instantiated was not updated: <a href="https://github.com/llvm/llvm-project/blob/470c454a6176ef31474553e408c90f5ee630df89/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp#L930-L931" target="_blank">https://github.com/llvm/llvm-project/blob/470c454a6176ef31474553e408c90f5ee630df89/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp#L930-L931</a><div><br></div><div>So I think this is an AST bug in the end, after all. I'll see if a patch breaks anything...</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Oct 28, 2021 at 11:18 PM Sam McCall <<a href="mailto:sammccall@google.com" target="_blank">sammccall@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Thu, Oct 28, 2021 at 11:08 PM Sam McCall <<a href="mailto:sammccall@google.com" target="_blank">sammccall@google.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>(Incidentally, I'm not sure why the derived class needs to be a template specialization for hasAnyBase() to match the injected-class-name, so still want to poke at this some more)</div></div></blockquote><div>Hmm, the reason is that in a template instantiation, the injected-class-name is a redecl ("prev 0x...." in the AST dump) of the ClassTemplateSpecializationDecl.</div><div>Whereas for a "normal" class this is not the case. This affects the behavior of hasDefinition() and others.</div><div><br></div><div>Maybe there's a good reason for this, but it seems very suspicious to me.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Oct 28, 2021 at 9:57 PM Sam McCall <<a href="mailto:sammccall@google.com" target="_blank">sammccall@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">My reading is:<div> - isVirtual() matcher indeed calls CXXMethodDecl::isVirtual</div><div> - CXXMethodDecl::isVirtual does what you expect for non-template code: has "virtual" specifier or overrides anything</div><div> - class template bodies don't contain implicit CXXDestructorDecls. These are only created when the class is instantiated, probably because in the general case that's when the analysis can happen. This is why the "unless" of your matcher does not apply.</div><div> - I'm confused about how your patch fixes this, because my reading of the code is that getDestructor() on the class template body should return null. Will dig into this a little.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Oct 28, 2021 at 9:05 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Thu, Oct 28, 2021 at 1:24 AM Carlos Galvez <<a href="mailto:carlosgalvezp@gmail.com" target="_blank">carlosgalvezp@gmail.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">> What does the original (buggy) implementation use instead of cxxDestructorDecl::isVirtual? <div><br><div>It used this matcher:</div><div><br></div><div> cxxRecordDecl(<br> anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),<br> unless(anyOf(<br> has(cxxDestructorDecl(isPublic(), isVirtual())),<br> has(cxxDestructorDecl(isProtected(), unless(isVirtual()))))))<br> .bind("ProblematicClassOrStruct"),<br></div><div><br></div><div>So the line:</div><div><br></div><div> has(cxxDestructorDecl(isPublic(), isVirtual())),<br></div><div><br></div><div>Should have done the job, but it didn't.</div></div></div></blockquote><div><br>That's the bit I'm trying to ask about/understand - do you know why "cxxDestructorDecl(isVirtual())" was inadequate, if "cxxDestructorDecl::isVirtual() (from Sema)" is adequate? I would've expected "cxxDestructorDecl(isVirtual())" to call CXXDestructorDecl::isVirtual - but I guess it doesn't?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><div><br></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Oct 25, 2021 at 7:06 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Sun, Oct 10, 2021 at 6:46 AM Carlos Galvez via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hi!<div><br></div><div>As proposed by @whisperity I'm posting here to ask about a strange situation we found in the following bug fix:</div><div><br></div><div><a href="https://reviews.llvm.org/D110614" target="_blank">https://reviews.llvm.org/D110614</a><br></div><div><br></div><div>We had a matcher to detect if a class has a public virtual destructor:</div><div><br></div><div><span style="color:rgb(0,0,0);font-family:monospace;font-size:13px;white-space:pre-wrap;background-color:rgba(71,87,120,0.1)">has(cxxDestructorDecl(isPublic(), isVirtual())),</span><br></div><div><br></div><div>However this didn't work for class templates, see details:</div><div><div><br></div><div><a href="https://reviews.llvm.org/D110614#3032760" target="_blank">https://reviews.llvm.org/D110614#3032760</a><br></div><div><br></div></div><div>The workaround was to create a custom matcher that seems to do the same thing as the original matcher, but it actually works for templates because it calls cxxDestructorDecl::isVirtual() (from Sema), which correctly propagates the virtual specifier from the base class.</div></div></blockquote><div><br>What does the original (buggy) implementation use instead of cxxDestructorDecl::isVirtual? If the original's just using the wrong query for testing virtuality that seems like a reasonable thing to fix as a bug?<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><br></div><div>Is this a concern, and should someone look into Sema/AST? (I'm unfortunately a n00b here). Or should we just merge the clang-tidy fix and move on?</div><div><br></div><div>Thanks!</div></div>
_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
</blockquote></div></div>
</blockquote></div>
</blockquote></div></div>
</blockquote></div>
</blockquote></div>
</blockquote></div></div>
</blockquote></div>
</blockquote></div>