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

[Sam McCall via cfe-dev]

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