[Lldb-commits] [PATCH] D75626: Add support for owning module information to TypeSystemClang.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

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).



================
Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:1211
+        *m_source_manager_up, *m_diagnostics_engine_up, *m_language_options_up,
+        m_target_info_up.get(), *m_header_search_up);
+  }
----------------
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.


================
Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h:310
   CompilerType CreateRecordType(clang::DeclContext *decl_ctx,
+                                unsigned owning_module,
                                 lldb::AccessType access_type,
----------------
Could we change this (and all similar parameters) from an `unsigned` to at one of those:
* A typedef such as `typedef unsigned ModuleID`. It seems to be optional, so `llvm::Optional<ModuleID>` would be even better so I could pass `llvm::None` instead of `0`.
* (preferred) A new `ModuleID` type that doesn't implicitly construct from `unsigned`. Many of these functions already take some kind of integer and I would prefer if we don't add more.

That would also make the `GetOrCreateClangModule` self-documenting.


================
Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h:500
+  /// Set the owning module for \p decl.
+  static void SetOwningModule(clang::Decl *decl, unsigned owning_module);
+
----------------
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).


================
Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h:1021
+  clang::SourceManager *GetSourceMgr() const {
+    return m_source_manager_up.get();
+  }
----------------
The correct way to get the SourceManager and LangOpts is to do `getASTContext().getSourceManager()`. `m_language_options_up`, `m_source_manager_up`etc.  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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75626/new/

https://reviews.llvm.org/D75626