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

[David Blaikie via cfe-dev]

On Thu, Oct 28, 2021 at 1:24 AM Carlos Galvez <carlosgalvezp at gmail.com>
wrote:

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

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?


>
>
> 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/adf2aa18/attachment.html>