<div dir="ltr">Sorry, should have done more digging first.<div><br></div><div>The matcher does match the main template class body as suspicious, but then the check() callback immediately to look up a destructor and fails, so class template bodies will never be warned on directly. Rather the false positive happens when the template is instantiated.</div><div><br></div><div>If you look carefully at the AST dump, there are *two* declarations of the specialization TemplatedDerived: a ClassTemplateSpecializationDecl, and a CXXRecordDecl redeclaration nested within it: the injected-class-name. The latter is what is being matched.</div><div>The injected-class-name is a simple redeclaration. It has no children of its own, so it never matches has() which just refers to AST structure. However if you do semantic lookups on it, it will find things in the "main" declaration. It matches InheritsVirtualMethod, because hasAnyBase is a semantic lookup. It doesn't match has(cxxDestructorDecl()) at all, but getDestructor() does work.</div><div>This is why your patch fixes that case, but it's pretty indirect. It'd be better to exclude the injected-class-name directly I suppose. Maybe by excluding isImplicit()?</div><div><br></div><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><br><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">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>