labath added a comment. This sounds like it could be useful though I can't say I really no much about modules.
I appreciate the effort in splitting this patch up, but I am wondering if we couldn't do this is one more step. Would it be possible to split the TypeSystem changes into a patch of its own (with some TypeSystemClang unit tests), and then have separate a patch where SymbolFileDWARF starts making use of this functionality? I think that would make easier to follow what's going in the two subsystems/plugins as well as reduce some of the noise caused by the interface changes. ================ Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:224 + if (auto *rd = llvm::dyn_cast<clang::RecordDecl>(tag_decl)) + for (auto *f : rd->fields()) + TypeSystemClang::SetOwningModule(f, owning_module); ---------------- Maybe spell out the type here? I have no idea what's the type of this.. ================ Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h:57-78 + /// The Layout is as follows: + /// \verbatim + /// bit 0..30 ... Owning Module ID. + /// bit 31 ...... IsCompleteObjCClass. + /// \endverbatim uint32_t &m_payload; public: ---------------- Maybe it would be simpler to have a struct full of bitfields and then just memcpy it to/from the uint32_t in the constructor/destructor? ================ Comment at: lldb/test/Shell/SymbolFile/DWARF/Inputs/ModuleOwnership/A.h:24 +extern template struct InNamespace<int>; +} ---------------- How about a case where A.h defines a type which references a type from B.h (e.g. contains it as a member variable) ? ================ Comment at: lldb/test/Shell/SymbolFile/DWARF/module-ownership.mm:1 +// RUN: %clang_host -g -gmodules -fmodules -fmodules-cache-path=%t.cache \ +// RUN: -c -o %t.o %s -I%S/Inputs ---------------- I think this test will fail on windows. At least it did fail for me when I manually forced a windows target here (linux was fine though). I think the problem is that lldb does not know how to open an unlinked COFF file. I think it would be better to hardcode a triple here instead of using %clang_host. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75488/new/ https://reviews.llvm.org/D75488 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
