[Lldb-commits] [PATCH] Introduce a TypeSystem interface to support adding non-clang languages.
Ryan Brown
ribrdb at google.com
Tue Mar 31 15:28:41 PDT 2015
On Mon, Mar 30, 2015 at 4:05 PM, Greg Clayton <clayborg at gmail.com> wrote:
> Overall a good start. I am confused as to why there were a lot of:
>
> clang_type.method(...)
>
> changed to
>
> type_system->method(clang_type, ...)
>
Well these are the methods which seemed too clang specific and I didn't
want to add them to the TypeSystem interface. Do you think these methods
are general to all type systems? Or do you think there's a better way to
handle these language specific methods?
I definitely agree it would be good to take the clang specific stuff out of
TypeSystem so other languages don't have to implement them.
I looked through all the TypeSystem methods:
~50 seem like they need to stay.
~20 are clang specific and probably would be better off in ClangASTContext.
~20 I'm not sure about
You can see the list in this spreadsheet:
https://docs.google.com/spreadsheets/d/1MAe6q5ZnHJ5kDTseq5GsG2dujigixaHiTTwReVOH0hs/edit?usp=sharing
What do you think is the best way to implement the casting?
I would probably just add a virtual AsXXX method to TypeSystem for each new
implementation.
That seems more straight forward than trying to support llvm::dyn_cast
> Seems like we should keep the old API as the ClangASTType contains a
> "TypeSystem*" and it would make the diffs a lot less noisy in
> SymbolFileDWARF. These are all of the "Why is this no longer a method on
> ..." inline code notations.
>
> It also seems like anything that is actually creating a ClangASTTypeSystem
> specific type should have the creation functions only in ClangASTTypeSystem
> and not make all of them pure virtual in the TypeSystem definition. Then
> each language will have its own TypeSystem subclass. See inlined notes that
> say "move to ClangASTTypeSystem.h?". If we do this, then each module needs
> to be able to get one of the TypeSystem specific subclasses from the module:
>
> ClangASTTypeSystem *
> Module::GetTypeSystemClang ();
>
> ClangASTTypeGo *
> Module::GetTypeSystemGo ();
>
> ClangASTTypeSwift *
> Module::GetTypeSystemSwift ();
>
> Then the DWARF parser would grab the right TypeSystem subclass from the
> module in SymbolFileDWARF and use that to create types. Each TypeSystem
> subclass can have its own way to create native types. All of which must
> return ClangASTType (this really should be renamed CompilerType) types.
>
> So the changes I would like to see is:
> 1 - Make ClangASTContext inherit from TypeSystem
> 2 - Try and isolate all of the language specific type creation routines
> into ClangASTContext (since it now inherits from TypeSystem) and let it
> create native clang types. This way not all languages have to make hundreds
> of pure virtual functions that they will never support. If any are generic
> enough, we can include them, but it would be cleaner to allow each langue
> to have its own language specific create/addField/addIVar, etc routines
> 3 - Add get_as<T>() to the TypeSystem and allow people to dyn_cast their
> TypeSystem into more specific subclasses where needed and where the pure
> virtual stuff falls down.
>
>
> REPOSITORY
> rL LLVM
>
> ================
> Comment at: include/lldb/Symbol/ClangASTContext.h:39
> @@ -37,3 +38,3 @@
>
> class ClangASTContext
> {
> ----------------
> Shouldn't this inherit from TypeSystem?
>
> ================
> Comment at: include/lldb/Symbol/ClangASTContext.h:58-59
> @@ -56,1 +57,4 @@
> +
> + ClangASTTypeSystem*
> + getTypeSystem();
>
> ----------------
> If ClangASTContext inherits from TypeSystem, this isn't needed.
>
> ================
> Comment at: include/lldb/Symbol/ClangASTContext.h:509
> @@ -490,2 +508,3 @@
> std::unique_ptr<clang::Builtin::Context> m_builtins_ap;
> + std::unique_ptr<ClangASTTypeSystem> m_type_system_ap;
> CompleteTagDeclCallback m_callback_tag_decl;
> ----------------
> Shouldn't ClangASTContext inherit from TypeSystem? If so, this isn't
> needed.
>
> ================
> Comment at: include/lldb/Symbol/ClangASTType.h:20
> @@ -19,1 +19,3 @@
>
> +class TypeSystem;
> +
> ----------------
> Put this in lldb-forward.h and remove any other forward references to
> TypeSystem.
>
> ================
> Comment at: include/lldb/Symbol/ClangASTType.h:38
> @@ -35,8 +37,3 @@
>
> //----------------------------------------------------------------------
> - ClangASTType (clang::ASTContext *ast_context, lldb::clang_type_t
> type) :
> - m_type (type),
> - m_ast (ast_context)
> - {
> - }
> -
> + ClangASTType (clang::ASTContext *ast_context, lldb::clang_type_t
> type);
> ClangASTType (clang::ASTContext *ast_context, clang::QualType
> qual_type);
> ----------------
> This should be "TypeSystem* type_system, void *type".
>
> ================
> Comment at: include/lldb/Symbol/ClangASTType.h:233-235
> @@ -235,2 +232,5 @@
>
> + clang::ASTContext *
> + GetASTContext() const;
> +
> ConstString
> ----------------
> We shouldn't need this we should just be able to use the GetTypeSystem()
> above. We might need to implement llvm style dyn_cast<> operators on
> TypeSystem.
>
> ================
> Comment at: include/lldb/Symbol/ClangASTType.h:264
> @@ -263,8 +263,3 @@
> void
> - SetClangType (clang::ASTContext *ast, lldb::clang_type_t type)
> - {
> - m_ast = ast;
> - m_type = type;
> - }
> -
> + SetClangType (clang::ASTContext *ast, lldb::clang_type_t type);
> void
> ----------------
> This should be "TypeSystem* type_system, void *type".
>
> ================
> Comment at: include/lldb/Symbol/TypeSystem.h:189-211
> @@ +188,25 @@
> +
> + virtual ClangASTType
> + AddConstModifier (void * type) const = 0;
> +
> + virtual ClangASTType
> + AddRestrictModifier (void * type) const = 0;
> +
> + virtual ClangASTType
> + AddVolatileModifier (void * type) const = 0;
> +
> + // Using the current type, create a new typedef to that type using
> "typedef_name"
> + // as the name and "decl_ctx" as the decl context.
> + virtual ClangASTType
> + CreateTypedefType (void * type, const char *typedef_name,
> + clang::DeclContext *decl_ctx) const = 0;
> +
> + virtual ClangASTType
> + GetArrayElementType (void * type, uint64_t *stride = nullptr) const =
> 0;
> +
> + virtual ClangASTType
> + GetCanonicalType (void * type) const = 0;
> +
> + virtual ClangASTType
> + GetFullyUnqualifiedType (void * type) const = 0;
> +
> ----------------
> move to ClangASTTypeSystem.h?
>
> We might not need these in the base class since they are used to create
> ClangASTTypeSystem based types. Each language should have their own
> creation types that not every language should have to implement.
>
> ================
> Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1715
> @@ -1713,8 +1714,3 @@
> {
> - return m_class_opaque_type.AddObjCClassProperty (m_property_name,
> -
> m_property_opaque_type,
> - m_ivar_decl,
> -
> m_property_setter_name,
> -
> m_property_getter_name,
> -
> m_property_attributes,
> -
> m_metadata_ap.get());
> + return
> static_cast<ClangASTTypeSystem*>(m_class_opaque_type.GetTypeSystem())->
> + AddObjCClassProperty (m_class_opaque_type.GetOpaqueQualType(),
> ----------------
> Do we want some sort of get_as<ClangASTTypeSystem>() to make sure we don't
> try this on another type system type?
>
> ================
> Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1817
> @@ -1813,2 +1816,3 @@
> ModuleSP module = GetObjectFile()->GetModule();
> + ClangASTTypeSystem* types =
> static_cast<ClangASTTypeSystem*>(class_clang_type.GetTypeSystem());
>
> ----------------
> Do we want some sort of get_as<ClangASTTypeSystem>() to make sure we don't
> try this on another type system type?
>
> ================
> Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1817
> @@ -1813,2 +1816,3 @@
> ModuleSP module = GetObjectFile()->GetModule();
> + ClangASTTypeSystem* types =
> static_cast<ClangASTTypeSystem*>(class_clang_type.GetTypeSystem());
>
> ----------------
> clayborg wrote:
> > Do we want some sort of get_as<ClangASTTypeSystem>() to make sure we
> don't try this on another type system type?
> We should make a common name for any TypeSystem variable like
> "type_system" and use these common names everywhere. "types" seems like the
> wrong variable name here for clarity sake.
>
> ================
> Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2006
> @@ -2001,4 +2005,3 @@
> accessibility = eAccessPublic;
> - class_clang_type.AddVariableToRecordType
> (name,
> -
> var_type->GetClangLayoutType(),
> -
> accessibility);
> + types->AddVariableToRecordType
> (class_clang_type.GetOpaqueQualType(),
> + name,
> ----------------
> Why is this no longer a method on "class_clang_type"?
>
> ================
> Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2124
> @@ -2118,3 +2123,3 @@
> {
> - clang::FieldDecl
> *unnamed_bitfield_decl = class_clang_type.AddFieldToRecordType (NULL,
> + clang::FieldDecl
> *unnamed_bitfield_decl = types->AddFieldToRecordType
> (class_clang_type.GetOpaqueQualType(), NULL,
>
>
> GetClangASTContext().GetBuiltinTypeForEncodingAndBitSize(eEncodingSint,
> word_width),
> ----------------
> Why is this no longer a method on "class_clang_type"?
>
> ================
> Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2174
> @@ -2168,3 +2173,3 @@
>
> - field_decl =
> class_clang_type.AddFieldToRecordType (name,
> + field_decl = types->AddFieldToRecordType
> (class_clang_type.GetOpaqueQualType(), name,
>
> member_clang_type,
> ----------------
> Why is this no longer a method on "class_clang_type"?
>
> ================
> Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2322
> @@ -2316,3 +2321,3 @@
> {
> -
> class_clang_type.SetObjCSuperClass(base_class_clang_type);
> +
> types->SetObjCSuperClass(class_clang_type.GetOpaqueQualType(),
> base_class_clang_type);
> }
> ----------------
> Why is this no longer a method on "class_clang_type"?
>
> ================
> Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2326
> @@ -2320,4 +2325,3 @@
> {
> - base_classes.push_back
> (base_class_clang_type.CreateBaseClassSpecifier (accessibility,
> -
> is_virtual,
> -
> is_base_of_class));
> + base_classes.push_back
> (types->CreateBaseClassSpecifier (base_class_clang_type.GetOpaqueQualType(),
> +
> accessibility,
> ----------------
> Why is this no longer a method on base_class_clang_type?
>
> ================
> Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2492
> @@ -2483,3 +2491,3 @@
> // clang::ExternalASTSource queries for this type.
> - clang_type.SetHasExternalStorage (false);
> + types->SetHasExternalStorage (clang_type.GetOpaqueQualType(), false);
>
> ----------------
> Why is this no longer a method on clang_type?
>
> ================
> Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2528
> @@ -2519,3 +2527,3 @@
> // the class is created.
> - clang_type.StartTagDeclarationDefinition ();
> + types->StartTagDeclarationDefinition
> (clang_type.GetOpaqueQualType());
> }
> ----------------
> Why no longer a method on the object like before?
>
> http://reviews.llvm.org/D8712
>
> EMAIL PREFERENCES
> http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20150331/342ef2bc/attachment.html>
More information about the lldb-commits
mailing list