[Lldb-commits] [PATCH] D67760: [lldb] Decouple importing the std C++ module from the way the program is compiled

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.


================
Comment at: lldb/include/lldb/Symbol/CompileUnit.h:228
 
+  /// Apply a lambda to each external module referenced by this compilation
+  /// unit. Recursively also descends into the referenced external modules
----------------
What does "external module" mean in this context? Can we be specific about lldb::Module vs clang::Module?


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp:442
+  std::string full_msg = "[C++ module config] " + msg;
+  LLDB_LOG(log, full_msg.c_str());
+  return CppModuleConfiguration();
----------------
if you use `LLDB_LOG(log, "[C++ module config] %s ", msg.c_str());` we only pay the cost for concatenating if logging is on.


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp:1
+//===-- CppModuleConfiguration.cpp ------------------------------*- C++ -*-===//
+//
----------------
the `-*- C++ -*-` only makes sense for `.h` files were it is ambiguous whether they contain C or C++ code.


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp:46
+  // we should remove that '/bits' suffix to get the actual include directory.
+  if (dir.endswith("/usr/include/bits"))
+    dir.consume_back("/bits");
----------------
llvm::sys::path::append to avoid hardcoding path separators?


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp:65
+    if (!analyzeFile(f))
+      break;
+
----------------

```
if (!std::all_of(supported_files.begin(), supported_files.end(), analyzeFile))
  return;
```
?


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp:68
+  // Calculate the resource directory for LLDB.
+  m_resource_inc = GetClangResourceDir().GetPath() + "/include";
+
----------------
sys::path::append


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h:28
+    /// True iff this path hasn't been set yet.
+    bool m_first = true;
+
----------------
micro-optimization: may use less space if the two bools are grouped together


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67760