[cfe-dev] [clang-tidy][Sema][AST] Different behavior Sema and AST matcher

[Carlos Galvez via cfe-dev]

> What does the original (buggy) implementation use instead of
cxxDestructorDecl::isVirtual?

It used this matcher:

      cxxRecordDecl(
          anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
          unless(anyOf(
              has(cxxDestructorDecl(isPublic(), isVirtual())),
              has(cxxDestructorDecl(isProtected(), unless(isVirtual()))))))
          .bind("ProblematicClassOrStruct"),

So the line:

              has(cxxDestructorDecl(isPublic(), isVirtual())),

Should have done the job, but it didn't.


On Mon, Oct 25, 2021 at 7:06 PM David Blaikie <dblaikie at gmail.com> wrote:

> On Sun, Oct 10, 2021 at 6:46 AM Carlos Galvez via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
>> Hi!
>>
>> As proposed by @whisperity I'm posting here to ask about a strange
>> situation we found in the following bug fix:
>>
>> https://reviews.llvm.org/D110614
>>
>> We had a matcher to detect if a class has a public virtual destructor:
>>
>> has(cxxDestructorDecl(isPublic(), isVirtual())),
>>
>> However this didn't work for class templates, see details:
>>
>> https://reviews.llvm.org/D110614#3032760
>>
>> 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.
>>
>
> 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?
>
>
>>
>> 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?
>>
>> Thanks!
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20211028/12b52841/attachment-0001.html>