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