[Lldb-commits] [PATCH] D39844: CompilerType: Add ability to retrieve an integral template argument

[Pavel Labath via Phabricator via lldb-commits]

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