<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Mar 30, 2015 at 4:05 PM, Greg Clayton <span dir="ltr"><<a href="mailto:clayborg@gmail.com" target="_blank">clayborg@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Overall a good start. I am confused as to why there were a lot of:<br>
<br>
clang_type.method(...)<br>
<br>
changed to<br>
<br>
type_system->method(clang_type, ...)<br></blockquote><div><br></div><div>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?</div><div><br></div><div>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.</div><div>I looked through all the TypeSystem methods:</div><div> ~50 seem like they need to stay.</div><div> ~20 are clang specific and probably would be better off in ClangASTContext.</div><div> ~20 I'm not sure about</div><div>You can see the list in this spreadsheet: <a href="https://docs.google.com/spreadsheets/d/1MAe6q5ZnHJ5kDTseq5GsG2dujigixaHiTTwReVOH0hs/edit?usp=sharing">https://docs.google.com/spreadsheets/d/1MAe6q5ZnHJ5kDTseq5GsG2dujigixaHiTTwReVOH0hs/edit?usp=sharing</a></div><div><br></div><div>What do you think is the best way to implement the casting?</div><div>I would probably just add a virtual AsXXX method to TypeSystem for each new implementation.</div><div>That seems more straight forward than trying to support llvm::dyn_cast</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
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.<br>
<br>
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:<br>
<br>
ClangASTTypeSystem *<br>
Module::GetTypeSystemClang ();<br>
<br>
ClangASTTypeGo *<br>
Module::GetTypeSystemGo ();<br>
<br>
ClangASTTypeSwift *<br>
Module::GetTypeSystemSwift ();<br>
<br>
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.<br>
<br>
So the changes I would like to see is:<br>
1 - Make ClangASTContext inherit from TypeSystem<br>
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<br>
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.<br>
<br>
<br>
REPOSITORY<br>
rL LLVM<br>
<br>
================<br>
Comment at: include/lldb/Symbol/ClangASTContext.h:39<br>
@@ -37,3 +38,3 @@<br>
<br>
class ClangASTContext<br>
{<br>
----------------<br>
Shouldn't this inherit from TypeSystem?<br>
<br>
================<br>
Comment at: include/lldb/Symbol/ClangASTContext.h:58-59<br>
@@ -56,1 +57,4 @@<br>
+<br>
+ ClangASTTypeSystem*<br>
+ getTypeSystem();<br>
<br>
----------------<br>
If ClangASTContext inherits from TypeSystem, this isn't needed.<br>
<br>
================<br>
Comment at: include/lldb/Symbol/ClangASTContext.h:509<br>
@@ -490,2 +508,3 @@<br>
std::unique_ptr<clang::Builtin::Context> m_builtins_ap;<br>
+ std::unique_ptr<ClangASTTypeSystem> m_type_system_ap;<br>
CompleteTagDeclCallback m_callback_tag_decl;<br>
----------------<br>
Shouldn't ClangASTContext inherit from TypeSystem? If so, this isn't needed.<br>
<br>
================<br>
Comment at: include/lldb/Symbol/ClangASTType.h:20<br>
@@ -19,1 +19,3 @@<br>
<br>
+class TypeSystem;<br>
+<br>
----------------<br>
Put this in lldb-forward.h and remove any other forward references to TypeSystem.<br>
<br>
================<br>
Comment at: include/lldb/Symbol/ClangASTType.h:38<br>
@@ -35,8 +37,3 @@<br>
//----------------------------------------------------------------------<br>
- ClangASTType (clang::ASTContext *ast_context, lldb::clang_type_t type) :<br>
- m_type (type),<br>
- m_ast (ast_context)<br>
- {<br>
- }<br>
-<br>
+ ClangASTType (clang::ASTContext *ast_context, lldb::clang_type_t type);<br>
ClangASTType (clang::ASTContext *ast_context, clang::QualType qual_type);<br>
----------------<br>
This should be "TypeSystem* type_system, void *type".<br>
<br>
================<br>
Comment at: include/lldb/Symbol/ClangASTType.h:233-235<br>
@@ -235,2 +232,5 @@<br>
<br>
+ clang::ASTContext *<br>
+ GetASTContext() const;<br>
+<br>
ConstString<br>
----------------<br>
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.<br>
<br>
================<br>
Comment at: include/lldb/Symbol/ClangASTType.h:264<br>
@@ -263,8 +263,3 @@<br>
void<br>
- SetClangType (clang::ASTContext *ast, lldb::clang_type_t type)<br>
- {<br>
- m_ast = ast;<br>
- m_type = type;<br>
- }<br>
-<br>
+ SetClangType (clang::ASTContext *ast, lldb::clang_type_t type);<br>
void<br>
----------------<br>
This should be "TypeSystem* type_system, void *type".<br>
<br>
================<br>
Comment at: include/lldb/Symbol/TypeSystem.h:189-211<br>
@@ +188,25 @@<br>
+<br>
+ virtual ClangASTType<br>
+ AddConstModifier (void * type) const = 0;<br>
+<br>
+ virtual ClangASTType<br>
+ AddRestrictModifier (void * type) const = 0;<br>
+<br>
+ virtual ClangASTType<br>
+ AddVolatileModifier (void * type) const = 0;<br>
+<br>
+ // Using the current type, create a new typedef to that type using "typedef_name"<br>
+ // as the name and "decl_ctx" as the decl context.<br>
+ virtual ClangASTType<br>
+ CreateTypedefType (void * type, const char *typedef_name,<br>
+ clang::DeclContext *decl_ctx) const = 0;<br>
+<br>
+ virtual ClangASTType<br>
+ GetArrayElementType (void * type, uint64_t *stride = nullptr) const = 0;<br>
+<br>
+ virtual ClangASTType<br>
+ GetCanonicalType (void * type) const = 0;<br>
+<br>
+ virtual ClangASTType<br>
+ GetFullyUnqualifiedType (void * type) const = 0;<br>
+<br>
----------------<br>
move to ClangASTTypeSystem.h?<br>
<br>
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.<br>
<br>
================<br>
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1715<br>
@@ -1713,8 +1714,3 @@<br>
{<br>
- return m_class_opaque_type.AddObjCClassProperty (m_property_name,<br>
- m_property_opaque_type,<br>
- m_ivar_decl,<br>
- m_property_setter_name,<br>
- m_property_getter_name,<br>
- m_property_attributes,<br>
- m_metadata_ap.get());<br>
+ return static_cast<ClangASTTypeSystem*>(m_class_opaque_type.GetTypeSystem())-><br>
+ AddObjCClassProperty (m_class_opaque_type.GetOpaqueQualType(),<br>
----------------<br>
Do we want some sort of get_as<ClangASTTypeSystem>() to make sure we don't try this on another type system type?<br>
<br>
================<br>
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1817<br>
@@ -1813,2 +1816,3 @@<br>
ModuleSP module = GetObjectFile()->GetModule();<br>
+ ClangASTTypeSystem* types = static_cast<ClangASTTypeSystem*>(class_clang_type.GetTypeSystem());<br>
<br>
----------------<br>
Do we want some sort of get_as<ClangASTTypeSystem>() to make sure we don't try this on another type system type?<br>
<br>
================<br>
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1817<br>
@@ -1813,2 +1816,3 @@<br>
ModuleSP module = GetObjectFile()->GetModule();<br>
+ ClangASTTypeSystem* types = static_cast<ClangASTTypeSystem*>(class_clang_type.GetTypeSystem());<br>
<br>
----------------<br>
clayborg wrote:<br>
> Do we want some sort of get_as<ClangASTTypeSystem>() to make sure we don't try this on another type system type?<br>
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.<br>
<br>
================<br>
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2006<br>
@@ -2001,4 +2005,3 @@<br>
accessibility = eAccessPublic;<br>
- class_clang_type.AddVariableToRecordType (name,<br>
- var_type->GetClangLayoutType(),<br>
- accessibility);<br>
+ types->AddVariableToRecordType (class_clang_type.GetOpaqueQualType(),<br>
+ name,<br>
----------------<br>
Why is this no longer a method on "class_clang_type"?<br>
<br>
================<br>
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2124<br>
@@ -2118,3 +2123,3 @@<br>
{<br>
- clang::FieldDecl *unnamed_bitfield_decl = class_clang_type.AddFieldToRecordType (NULL,<br>
+ clang::FieldDecl *unnamed_bitfield_decl = types->AddFieldToRecordType (class_clang_type.GetOpaqueQualType(), NULL,<br>
GetClangASTContext().GetBuiltinTypeForEncodingAndBitSize(eEncodingSint, word_width),<br>
----------------<br>
Why is this no longer a method on "class_clang_type"?<br>
<br>
================<br>
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2174<br>
@@ -2168,3 +2173,3 @@<br>
<br>
- field_decl = class_clang_type.AddFieldToRecordType (name,<br>
+ field_decl = types->AddFieldToRecordType (class_clang_type.GetOpaqueQualType(), name,<br>
member_clang_type,<br>
----------------<br>
Why is this no longer a method on "class_clang_type"?<br>
<br>
================<br>
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2322<br>
@@ -2316,3 +2321,3 @@<br>
{<br>
- class_clang_type.SetObjCSuperClass(base_class_clang_type);<br>
+ types->SetObjCSuperClass(class_clang_type.GetOpaqueQualType(), base_class_clang_type);<br>
}<br>
----------------<br>
Why is this no longer a method on "class_clang_type"?<br>
<br>
================<br>
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2326<br>
@@ -2320,4 +2325,3 @@<br>
{<br>
- base_classes.push_back (base_class_clang_type.CreateBaseClassSpecifier (accessibility,<br>
- is_virtual,<br>
- is_base_of_class));<br>
+ base_classes.push_back (types->CreateBaseClassSpecifier (base_class_clang_type.GetOpaqueQualType(),<br>
+ accessibility,<br>
----------------<br>
Why is this no longer a method on base_class_clang_type?<br>
<br>
================<br>
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2492<br>
@@ -2483,3 +2491,3 @@<br>
// clang::ExternalASTSource queries for this type.<br>
- clang_type.SetHasExternalStorage (false);<br>
+ types->SetHasExternalStorage (clang_type.GetOpaqueQualType(), false);<br>
<br>
----------------<br>
Why is this no longer a method on clang_type?<br>
<br>
================<br>
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2528<br>
@@ -2519,3 +2527,3 @@<br>
// the class is created.<br>
- clang_type.StartTagDeclarationDefinition ();<br>
+ types->StartTagDeclarationDefinition (clang_type.GetOpaqueQualType());<br>
}<br>
----------------<br>
Why no longer a method on the object like before?<br>
<br>
<a href="http://reviews.llvm.org/D8712" target="_blank">http://reviews.llvm.org/D8712</a><br>
<br>
EMAIL PREFERENCES<br>
<a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<br>
</blockquote></div><br></div></div>