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

[Carlos Galvez via cfe-dev]

Wow, amazing job figuring this out thanks a lot! I wonder if that will fix
other problems related to templates, for example this one:
https://bugs.llvm.org/show_bug.cgi?id=51802

(The template in the constructor is irrelevant)

Can't wait to try it out :)

On Fri, Oct 29, 2021 at 12:58 AM Sam McCall <sammccall at google.com> wrote:

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