[Lldb-commits] [PATCH] D147248: [lldb] Use one Progress event per root module build

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp:228
 void StoringDiagnosticConsumer::SetCurrentModuleProgress(
-    llvm::StringRef module_name) {
-  // Ensure the ordering of:
-  //   1. Completing the existing progress event.
-  //   2. Beginining a new progress event.
-  m_current_progress_up = nullptr;
-  m_current_progress_up = std::make_unique<Progress>(
-      llvm::formatv("Currently building module {0}", module_name));
+    const std::string &module_name) {
+  if (!m_current_progress_up)
----------------
JDevlieghere wrote:
> kastiglione wrote:
> > JDevlieghere wrote:
> > > `Progress::Increment` is taking a `std::string` by value, so if you think this will ever get called with an r-value reference, you should do the same and `std::move` it. Otherwise the StringRef or the `const std::string&` both require a copy.
> > Understood. The module names are references, I could change this function to take a `string &&`, but that would move the copy to the callee rather than here. Either way, I don't see a way to avoid a copy.
> Yeah if you don't have an r-value reference it cannot be avoided, but I think it's still best to make this take it by value so in case there ever is one in the caller to this function, it is avoided. 
Take it by value, or by r-value?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147248