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

[Sam McCall via cfe-dev]

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

> (Incidentally, I'm not sure why the derived class needs to be a template
> specialization for hasAnyBase() to match the injected-class-name, so still
> want to poke at this some more)
>
Hmm, the reason is that in a template instantiation, the
injected-class-name is a redecl ("prev 0x...." in the AST dump) of the
ClassTemplateSpecializationDecl.
Whereas for a "normal" class this is not the case. This affects the
behavior of hasDefinition() and others.

Maybe there's a good reason for this, but it seems very suspicious to me.

On Thu, Oct 28, 2021 at 9:57 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/ca0e4dcf/attachment-0001.html>