[Lldb-commits] [PATCH] D58125: Add ability to import std module into expression parser to improve C++ debugging

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thank you for writing the test. I think it's really great that we're able to test something like this without depending on the actual c++ library existing and supporting modules. (I think we should have more tests like that).



================
Comment at: lldb/include/lldb/Expression/ExpressionSourceCode.h:55
+               bool static_method, ExecutionContext &exe_ctx, bool add_locals,
+               std::vector<std::string> modules) const;
 
----------------
`ArrayRef<std::string>` ?


================
Comment at: lldb/packages/Python/lldbsuite/test/make/Makefile.rules:257
 
-CFLAGS += -I$(SRCDIR) -include $(THIS_FILE_DIR)test_common.h -I$(THIS_FILE_DIR)
+ifndef NO_INC_DIRS
+  CFLAGS += -I$(SRCDIR) -include $(THIS_FILE_DIR)test_common.h -I$(THIS_FILE_DIR)
----------------
I'm wondering why have you needed to do this (and whether we can figure out another solution to that).


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp:254
+  std::vector<std::string> system_include_directories =
+      Platform::GetHostPlatform()->GetSystemIncludeDirectories(
+          lldb::eLanguageTypeC_plus_plus);
----------------
Shouldn't this use the platform of the currently executed target (instead of the host platform)?


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp:273
     : ExpressionParser(exe_scope, expr, generate_debug_info), m_compiler(),
-      m_pp_callbacks(nullptr) {
+      m_pp_callbacks(nullptr), m_include_directories(include_directories) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
----------------
`m_include_directories(std::move(include_directories))`


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp:479-489
+      llvm::SmallString<64> path_string;
+      llvm::raw_svector_ostream stream(path_string);
+      for (const ConstString &s : m.path)
+        stream << s.AsCString() << '.';
+
+      // Drop trailing '.'.
+      if (!path_string.empty())
----------------
all of this can boil down to something like:
`LLDB_LOG(log, "Found module in compile unit: {0} - include dir: {1}", llvm::make_range(m.path.begin(), m.path.end()), m.search_path);`

(i'd recommend using LLDB_LOG elsewhere too, but here the benefit is really obvious).


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp:534-539
+    llvm::SmallString<64> modules_list;
+    llvm::raw_svector_ostream stream(modules_list);
+    for (const std::string &module : modules_to_include)
+      stream << " " << module << "\n";
+    log->Printf("List of imported modules in expression:\n%sEnd of list",
+                modules_list.c_str());
----------------
LLDB_LOG


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

https://reviews.llvm.org/D58125