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

[David Blaikie via cfe-dev]

Thanks for the summary/jumping in here Sam - curious to hear what comes up,
but glad it's in your hands/someone with more context here!

On Thu, Oct 28, 2021 at 12:58 PM Sam McCall <sammccall at google.com> wrote:

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