labath added a comment. In https://reviews.llvm.org/D39844#920621, @clayborg wrote:
> Just a few changes. > > - I would like the see the SBType returned for the integer template types as > it is what I would expect to happen. I tried to explain my reasoning in the inline comments. I don't feel strongly about that, but I do fell that makes a better API (it also mirrors what the underlying clang class does). > - we should add default implementations for stuff in TypeSystem and not > require all languages to override everything. I know this isn't the way > things were done in the past, but we should fix that to avoid bloat in all > TypeSystem subclasses. Good idea. Will do. ================ Comment at: include/lldb/Symbol/TypeSystem.h:356-360 + virtual CompilerType GetTypeTemplateArgument(lldb::opaque_compiler_type_t type, + size_t idx) = 0; + virtual std::pair<llvm::APSInt, CompilerType> + GetIntegralTemplateArgument(lldb::opaque_compiler_type_t type, + size_t idx) = 0; ---------------- clayborg wrote: > Why not make three functions: > - get kind by index > - get type by index > - get integer by index > > The "get integer by index" would only work if the kind was > eTemplateArgumentKindIntegral? Not sure why we have the pair getter? The second element of the pair is the *type* of the integral parameter (so you can e.g. differentiate between `foo<47>` and `foo<47ULL>`). I don't really have a use for it now, but it made sense to me to return this way (instead of through the "get type by index" function). ================ Comment at: source/API/SBType.cpp:422 + CompilerType template_arg_type = + m_opaque_sp->GetCompilerType(false).GetTypeTemplateArgument(idx); + if (template_arg_type.IsValid()) ---------------- clayborg wrote: > labath wrote: > > I should point out that this changes the behaviour of > > `SBType::GetTemplateArgumentType` slightly. > > > > Previously, for an integral template argument (e.g. `foo<47>`), it returned > > the *type* of the integer argument (i.e. `int`), where as now it will > > return nothing. > > > > I think this is the behavior that makes most sense for the underlying API, > > but if you're worried about compatibility, I can add a special case here (I > > am not worried as I don't think there is anything reasonable the user could > > have done with the returned value anyway). > I would like this to return the type here as I believe that makes the most > sense. Why would we not return the correct type of the template argument for > integers? Well.. I am not sure what would the SBType users expect, but I looked at all the in-tree CompilerType users. All of them are data formatters and they use it to pluck the element type out of e.g. `std::vector<int>`. If somebody declares a type `std::vector<47>` for whatever reason, we wouldn't be doing them a favor by returning `int` to their question "what is the first template argument?". Some of the formatters (e.g. LibCxxInitializerList) actually check that the returned kind is `Type`, but most of them just take the result as face value. By returning the type only if it was a type template, we are making the formatters more correct and simpler. That was my reasoning for CompilerType. I am not really sure how SBType is used out there, but my expectation is that it will be used in a similar way as we use CompilerType. I can see the logic in the other approach as well, but right now I can't think of any situation where I'd want to treat `foo<int>` and `foo<47>` in the same way. https://reviews.llvm.org/D39844 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
