[Lldb-commits] [PATCH] D18381: Decouple ClangASTContext from DWARF

[Zachary Turner via lldb-commits]

zturner updated this revision to Diff 51476.
zturner added a comment.

This patch has gotten both bigger and smaller.  Smaller in the sense that after taking into consideration the most recent comments, there was really only one piece left of my patch.  It is the piece that abstracted out `CanComplete`, `CompleteType`, and `LayoutRecord` into a new class called `ClangTypeImportHelper` so that the same code could be re-used by `PDBASTParser`.

But upon closer inspection, this code ultimately just contained multiple levels of indirection to do what seems to be better housed inside of `ClangASTImporter`.  I mean, `ClangASTImporter::Import()`, makes sense right?  Physically, there's no real different in doing it this way versus the other way.  In both cases the `DWARFASTParserClang` had an instance of the thing.  Both the `ClangASTImporter` and the `ClangTypeImportHelper` with my first patch, and just a `ClangASTImporter` with my second patch.  But the code is much cleaner and more straightforward now, with fewer ping-ponging of API calls to get to the right place now that those 3 functions live directly isnide of `ClangASTImporter`.

In doing so though, I had some difficulty with the circular dependency relationship between `ClangASTImporter` and `ClangASTContext`.  They were each calling into each other in some cases, and this was leaking into header files causing compiler errors.  To address this I identified the code that needed to be shared (a couple of static methods in `ClangASTContext`) and moved them into a new file called `ClangUtil`.

Unfortunately this dirties up the CL and makes it look really large, even though a lot of the changes are just namespace fixes.

There are still many many instances of this kind of thing remaining in `ClangASTContext` which i did not address (because it quickly would have made the CL un-reviewable), but I would like to continue cleaning this up in subsequent patches.  `ClangASTContext.cpp` is a massive file, and anything we can do to split this code up will help people maintain this code (not to mention help new people grok it).  Plus removing circular dependencies is always a win.


http://reviews.llvm.org/D18381

Files:
  include/lldb/Symbol/ClangASTContext.h
  include/lldb/Symbol/ClangASTImporter.h
  include/lldb/Symbol/ClangUtil.h
  include/lldb/Symbol/TypeSystem.h
  source/Plugins/ExpressionParser/Clang/ASTDumper.cpp
  source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
  source/Plugins/ExpressionParser/Clang/IRForTarget.cpp
  source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.cpp
  source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTypeEncodingParser.cpp
  source/Plugins/SymbolFile/DWARF/DWARFASTParser.h
  source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  source/Symbol/CMakeLists.txt
  source/Symbol/ClangASTContext.cpp
  source/Symbol/ClangASTImporter.cpp
  source/Symbol/ClangUtil.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D18381.51476.patch
Type: text/x-patch
Size: 89596 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20160323/429397ec/attachment-0001.bin>