Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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, ...)
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.
| include/lldb/Symbol/ClangASTContext.h | ||
|---|---|---|
| 39 ↗ | (On Diff #22911) | Shouldn't this inherit from TypeSystem? |
| 58–59 ↗ | (On Diff #22911) | If ClangASTContext inherits from TypeSystem, this isn't needed. |
| 509 ↗ | (On Diff #22911) | Shouldn't ClangASTContext inherit from TypeSystem? If so, this isn't needed. |
| include/lldb/Symbol/ClangASTType.h | ||
| 20 ↗ | (On Diff #22911) | Put this in lldb-forward.h and remove any other forward references to TypeSystem. |
| 38 ↗ | (On Diff #22911) | This should be "TypeSystem* type_system, void *type". |
| 233–235 ↗ | (On Diff #22911) | 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. |
| 264 ↗ | (On Diff #22911) | This should be "TypeSystem* type_system, void *type". |
| include/lldb/Symbol/TypeSystem.h | ||
| 189–211 ↗ | (On Diff #22911) | 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. |
| source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp | ||
| 1715 ↗ | (On Diff #22911) | Do we want some sort of get_as<ClangASTTypeSystem>() to make sure we don't try this on another type system type? |
| 1817 ↗ | (On Diff #22911) | Do we want some sort of get_as<ClangASTTypeSystem>() to make sure we don't try this on another type system type? |
| 1817 ↗ | (On Diff #22911) | 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. |
| 2006 ↗ | (On Diff #22911) | Why is this no longer a method on "class_clang_type"? |
| 2124 ↗ | (On Diff #22911) | Why is this no longer a method on "class_clang_type"? |
| 2174 ↗ | (On Diff #22911) | Why is this no longer a method on "class_clang_type"? |
| 2322 ↗ | (On Diff #22911) | Why is this no longer a method on "class_clang_type"? |
| 2326 ↗ | (On Diff #22911) | Why is this no longer a method on base_class_clang_type? |
| 2492 ↗ | (On Diff #22911) | Why is this no longer a method on clang_type? |
| 2528 ↗ | (On Diff #22911) | Why no longer a method on the object like before? |
I think I've addressed most of your comments, except removing more methods from TypeSystem.h. I've added TODOs for the methods I think should be removed, and the ones I'm unsure about.
I've also changed the style for the ClangASTContext specific methods.
Instead of ast->Foo(type.GetOpaqueQualType()), it's now ClangASTContext::Foo(type). I think this is cleaner and safer.
If you like this style and agree on which methods to keep I'll go ahead and move the methods.
Looks great. Very few changes needed. See inline comments.
| include/lldb/Symbol/ClangASTType.h | ||
|---|---|---|
| 443–447 ↗ | (On Diff #23189) | This shouldn't be here, it should probably be moved as a static function to where the above functions moved. |
| include/lldb/lldb-forward.h | ||
| 225 ↗ | (On Diff #23189) | Fix spacing to match. If this is a tab, this should be spaces as we don't use tabs in LLDB sources. |
Please take another look. I've moved around some methods between ClangASTType, ClangASTContext, and TypeSystem based on trying to implement a TypeSystem for go.
Update to top of tree and you should be good to go as long as the test suite passes everything.
Looks like your CL caused some test regression - http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/3040
Please fix or revert the CL.
Oops, I guess building the "run testsuite" scheme in xcode doesn't actually run the tests.
This version fixes the tests on my linux machine.
Committed an updated version of this with the following two revisions:
Committed revision 244679.
Committed revision 244681.