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
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
