[PATCH] D140423: [WIP][clang] Add PrintingPolicy callback for identifying default template arguments

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D140423#4055722 <https://reviews.llvm.org/D140423#4055722>, @Michael137 wrote:

> In D140423#4052442 <https://reviews.llvm.org/D140423#4052442>, @aaron.ballman wrote:
>
>> In D140423#4052271 <https://reviews.llvm.org/D140423#4052271>, @dblaikie wrote:
>>
>>> In D140423#4051262 <https://reviews.llvm.org/D140423#4051262>, @aaron.ballman wrote:
>>>
>>>>> Add something like a bool IsDefaulted somewhere in Clang, e.g., in TemplateArgument and consult it from the TypePrinter. This would be much simpler but requires adding a field on one of the Clang types
>>>>
>>>> I think this might be worth exploring as a cleaner solution to the problem. `TemplateArgument` has a union of structures for the various kinds of template arguments it represents (https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/TemplateBase.h#L140). All of the structures in that union start with an `unsigned Kind` field to discriminate between the members. There are only 8 kinds currently (https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/TemplateBase.h#L63), so we could turn `Kind` into a bit-field and then steal a bit for `IsDefaulted` without increasing memory overhead. Do you think that's a reasonable approach to try instead?
>>>
>>> FWIW, I Think I discouraged @Michael137 from going down that direction since it felt weird to me to add that sort of thing to the Clang ASTs for an lldb-only use case, and a callback seemed more suitable. But this is hardly my wheelhouse - if you reckon that's the better direction, carry on, I expect @Michael137 will be on board with that.
>>
>> Adding in @erichkeane as templates code owner in case he has opinions.
>>
>> I agree it's a bit weird to modify the AST only for lldb only, but adding a callback to the printing policy is basically an lldb-only change as well (I don't imagine folks would use that callback all that often). So my thinking is that if we can encode the information in the AST for effectively zero cost, that helps every consumer of the AST (thinking about things like clang-tidy) and not just people printing. However, this is not a strongly held position, so if there's a preference for the current approach, it seems workable to me.
>
> Thanks for taking a look @aaron.ballman @erichkeane
>
> I prepared an alternative draft patch series with your suggestion of adding an `IsDefaulted` bit to `TemplateArgument`:
>
> - https://reviews.llvm.org/D141826
> - https://reviews.llvm.org/D141827
>
> Is this what you had in mind?
>
> This *significantly* simplifies the LLDB support: https://reviews.llvm.org/D141828
>
> So I'd prefer this over the callback approach TBH.
>
>> A Class template instantiation SHOULD have its link back to the class template, and should be able to calculate whether the template argument is defaulted, right? At least if it is the SAME as the default (that is, I'm not sure how well we can tell the difference between a defaulted arg, and a arg set to the default value).
>>
>> I wouldn't expect a bool to be necessary, and I see isSubstitutedDefaultArgument seems to do that work, right?
>
> As @dblaikie mentioned, unfortunately LLDB currently isn't able to construct the `ClassTemplateDecl` in a way where `isSubstitutedDefaultArgument` would correctly deduce whether a template instantiation has defaulted arguments. In DWARF we only have info about a template instantiation (i.e., the structure of the generic template parameters is not encoded). So we can't supply `(Non)TypeTemplateParamDecl::setDefaultArgument` with the generic arguments Clang expects. The `ClassTemplateDecl` parameters are set up here: https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp#L1391-L1402
>
> Regardless of how complex the template parameters get, LLDB just turns each into a plain `(Non)TypeTemplateParamDecl`

Got it, I missed that part, I'm not too familiar with how LLDB works in this case.  I'll take a look at those other patches.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140423/new/

https://reviews.llvm.org/D140423