<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><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">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>