This was split out from https://reviews.llvm.org/D75488.
Diff Detail
Event Timeline
Thanks for splitting this up. This seems fine to me. I'll leave the approval to the type system folks.
I don't know how much we care about the size of the Type class. If we did, there are ways this could be optimized. But I guess the fact that we stored m_is_complete_objc_class as a lone bool flag (and so changing that to a uint32_t doesn't cost anything) means that we don't really care that much about it.
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp | ||
---|---|---|
1445–1446 | bad formatting | |
7621 | here too |
I'm not a big fan of storing data in the ClangExternalASTSourceCallbacks. The main reason is that this external source only exists when the ASTContext in the TypeSystemClang is created by the TypeSystemClang. When the TypeSystem adopts an ASTContext there can be any (or none) ExternalASTSource so GetOrCreateClangModule would crash/assert. ClangExternalASTSourceCallbacks always knows its respective TypeSystemClang, so storing the data in TypeSystemClang and then getting it from there when getSourceDescriptor/getModule is called will always work (and saves you from RTTI-casting/checking the ExternalASTSource).
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp | ||
---|---|---|
1211 | All the m_* variables that correspond to a part of ASTContext are only initialized when the TypeSystemClang has created its own ASTContext, so this code would crash if that's not the case. Calling getASTContext().getX() is the way that always works. | |
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h | ||
323 | Could we change this (and all similar parameters) from an unsigned to at one of those:
That would also make the GetOrCreateClangModule self-documenting. | |
513 | Could this be declared next to GetOrCreateClangModule? This is currently declared in the middle of the CompilerDeclContext backend (which no longer makes so much sense with the new version of the patch). | |
1038 | The correct way to get the SourceManager and LangOpts is to do getASTContext().getSourceManager(). m_language_options_up, m_source_manager_upetc. are only set if the TypeSystemClang created its own ASTContext but are null if the TypeSystemClang adopted an ASTContext (e.g., in an expression). This can also return a reference instead of a pointer. I removed the option to have an empty TypeSystemClang. |
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h | ||
---|---|---|
89 | Why not use uint32_t like we did above? If we are going to assume 32 bits we should just use the fixed width type. |
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp | ||
---|---|---|
996 ↗ | (On Diff #248234) | We are passing around 0 everywhere and it is not obvious what it means at all. Can we name this somehow or make it a type? |
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h | ||
---|---|---|
146 | I am kinda lost at this point, so I'm not sure which patch introduces this, but... this name does not seem very ideal, as it sounds like it is related to lldb_private::Module, while it does not really have that much in common with it. And then there's ClangModulesDeclVendor::ModuleID -- I have no idea what's the relationship of this to that.. |
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp | ||
---|---|---|
996 ↗ | (On Diff #248234) | This is just an intermediate step and gets replaced in https://reviews.llvm.org/D75488 |
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h | ||
146 | I agree. I'm going to clean this up some more and I will also merge this review into https://reviews.llvm.org/D75488 because it doesn't actually make reviewing easier to see these API changes without their use. | |
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h | ||
89 | That is consistent with the type Clang uses for module IDs. |
And then there's ClangModulesDeclVendor::ModuleID -- I have no idea what's the relationship of this to that..
ClangModulesDeclVendor::ModuleID is a typedef to clang::Module * so it is a bit of a misnomer, since clang calls the unsigned module numbers module ids.
I am kinda lost at this point, so I'm not sure which patch introduces this, but... this name does not seem very ideal, as it sounds like it is related to lldb_private::Module, while it does not really have that much in common with it.
And then there's ClangModulesDeclVendor::ModuleID -- I have no idea what's the relationship of this to that..