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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
