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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl abandoned this revision.
aprantl marked an inline comment as done.
aprantl added inline comments.


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:996
       m_clang_ast_context->GetUniqueNamespaceDeclaration(
-          g_lldb_local_vars_namespace_cstr, nullptr);
+          g_lldb_local_vars_namespace_cstr, nullptr, 0);
   if (!namespace_decl)
----------------
shafik wrote:
> 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?
This is just an intermediate step and gets replaced in https://reviews.llvm.org/D75488


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h:146
                                                        DWARFDIE *decl_ctx_die);
+  lldb_private::ModuleID GetOwningModuleID(const DWARFDIE &die);
 
----------------
labath wrote:
> 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..
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.


================
Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h:73
+  unsigned GetOwningModuleID() { return Flags(m_payload).Clear(ObjCClassBit); }
+  void SetOwningModuleID(unsigned id) {
+    assert(id < ObjCClassBit);
----------------
shafik wrote:
> 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.
That is consistent with the type Clang uses for module IDs.


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

https://reviews.llvm.org/D75626