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

[Sam McCall via cfe-dev]

My reading is:
 - isVirtual() matcher indeed calls CXXMethodDecl::isVirtual
 - CXXMethodDecl::isVirtual does what you expect for non-template code: has
"virtual" specifier or overrides anything
 - 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.
 - 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.

On Thu, Oct 28, 2021 at 9:05 PM David Blaikie <dblaikie at gmail.com> wrote:

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