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

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

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.
- 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.



================
Comment at: include/lldb/Symbol/GoASTContext.h:318-334
+  lldb::TemplateArgumentKind
+  GetTemplateArgumentKind(lldb::opaque_compiler_type_t type,
+                          size_t idx) override {
+    return lldb::eTemplateArgumentKindNull;
+  }
+
+  CompilerType GetTypeTemplateArgument(lldb::opaque_compiler_type_t type,
----------------
I know this isn't how things were done up to now, but we really should move these default implementations in the original class and not require everyone to override with a version that does nothing. Can we not make these pure virtual in the original class?


================
Comment at: include/lldb/Symbol/JavaASTContext.h:206-220
+  lldb::TemplateArgumentKind
+  GetTemplateArgumentKind(lldb::opaque_compiler_type_t type,
+                          size_t idx) override {
+    return lldb::eTemplateArgumentKindNull;
+  }
+
+  CompilerType GetTypeTemplateArgument(lldb::opaque_compiler_type_t type,
----------------
I know this isn't how things were done up to now, but we really should move these default implementations in the original class and not require everyone to override with a version that does nothing. Can we not make these pure virtual in the original class?


================
Comment at: include/lldb/Symbol/OCamlASTContext.h:234-248
+  lldb::TemplateArgumentKind
+  GetTemplateArgumentKind(lldb::opaque_compiler_type_t type,
+                          size_t idx) override {
+    return lldb::eTemplateArgumentKindNull;
+  }
+
+  CompilerType GetTypeTemplateArgument(lldb::opaque_compiler_type_t type,
----------------
I know this isn't how things were done up to now, but we really should move these default implementations in the original class and not require everyone to override with a version that does nothing. Can we not make these pure virtual in the original class?


================
Comment at: include/lldb/Symbol/TypeSystem.h:354-360
+  virtual lldb::TemplateArgumentKind
+  GetTemplateArgumentKind(lldb::opaque_compiler_type_t type, size_t idx) = 0;
+  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;
----------------
Put default implementations here, and don't modify the Go, Java and OCaml ASTs?


================
Comment at: source/API/SBType.cpp:422
+  CompilerType template_arg_type =
+      m_opaque_sp->GetCompilerType(false).GetTypeTemplateArgument(idx);
+  if (template_arg_type.IsValid())
----------------
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?


https://reviews.llvm.org/D39844