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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
