[Lldb-commits] [PATCH] D61606: Add support for importing general C++ modules into the LLDB expression evaluator

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.


================
Comment at: lldb/include/lldb/Symbol/CompileUnit.h:397
   std::vector<SourceModule> m_imported_modules;
+  /// All modules, including the current module, used directly or indirectly
+  /// by this compile unit.
----------------
All *Clang/Source* modules? Otherwise it sounds like lldb::Modules.


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp:486
+    if (!m.search_path.IsEmpty())
+      m_include_directories.push_back(m.search_path);
+  }
----------------
std::copy_if?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:933
+      // Clang emits some modulemap files as module tags, so we have to
+      // filter them out here as they are not actual SourceModules.
+      if (name == "module.modulemap")
----------------
Is that a bug in Clang? Can you send me an example?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:934
+      // filter them out here as they are not actual SourceModules.
+      if (name == "module.modulemap")
+        continue;
----------------
That is not the only valid name for module map files, we also support module.map, module.private.modulemap and possibly more. I feel like there should be a better solution to the underlying problem.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:995
+
+    // The first compile unit should contain all used modules.
+    if (module->GetNumCompileUnits() == 0)
----------------
This sounds wrong for LTO binaries, which will contain many Clang CUs in one file.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D61606