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

[Sam McCall via cfe-dev]

Sent https://reviews.llvm.org/D112765.

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

> Looks like originally injected-class-names were considered redecls, then
> this was changed for CXXRecordDecl in 2010:
> https://github.com/llvm/llvm-project/commit/470c454a6176ef31474553e408c90f5ee630df89
> but the corresponding place where templates were instantiated was not
> updated:
> https://github.com/llvm/llvm-project/blob/470c454a6176ef31474553e408c90f5ee630df89/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp#L930-L931
>
> So I think this is an AST bug in the end, after all. I'll see if a patch
> breaks anything...
>
> On Thu, Oct 28, 2021 at 11:18 PM Sam McCall <sammccall at google.com> wrote:
>
>> 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/20211029/161bd877/attachment-0001.html>