[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.

Thanks Raphael. Apart from some inline nits, I have no further comments from my side, but hopefully someone with more familiarity with modules and clang ASTs can give this the final pass.



================
Comment at: lldb/include/lldb/Expression/ExpressionSourceCode.h:13-16
+#include "llvm/ADT/ArrayRef.h"
 
 #include <string>
+#include <vector>
----------------
It looks like these includes are no longer needed.


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp:279
+    std::string module_imports;
+    for (std::string module : modules) {
+      module_imports.append("@import ");
----------------
`const std::string &` avoids a copy


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp:480-481
+    for (const SourceModule &m : sc.comp_unit->GetImportedModules()) {
+      // FIXME: Replace make_range with join(begin(), end(), ".") once join
+      // supports ConstString.
+      LLDB_LOG(log, "Found module in compile unit: {0} - include dir: {1}",
----------------
I would actually say that make_range is better than join here, because it avoids constructing a temporary string. So, I'd just remove the FIXME here. (BTW, if you want to control the separator string, there's already a syntax for that: `{0:$[.]}`)


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

https://reviews.llvm.org/D58125