Michael137 added a comment. 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 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 this `isSubstitutedDefaultArgument` would correctly deduce whether a template instantiation has defaulted arguments. In DWARF we only have info about a template instantiation, but 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` 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
