On Mon, Mar 30, 2015 at 4:05 PM, Greg Clayton <[email protected]> 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/ > > >
_______________________________________________ lldb-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
