teemperor marked 6 inline comments as done.
teemperor added a comment.

Thanks for the quick review!



================
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
----------------
aprantl wrote:
> What does "external module" mean in this context? Can we be specific about 
> lldb::Module vs clang::Module?
Rather lldb::Module. Well, it refers to the external type modules we get with 
-gmodules (i.e. what we store in SymbolFileDWRF::m_external_type_modules), so 
it is kind of both in this case.


================
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");
----------------
aprantl wrote:
> llvm::sys::path::append to avoid hardcoding path separators?
I would rather have this converted to POSIX style and work with that. Otherwise 
this whole function needs to be path-sep independent (including escaping the 
regex and what not). And that ends up being a lot of boilerplate to make this 
OS-specific code OS-agnostic.


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

https://reviews.llvm.org/D67760



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to