[Lldb-commits] [PATCH] D140056: [lldb] Report clang module build remarks

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp:198
+    // End the previous event before starting a new event.
+    m_current_progress_up.reset(nullptr);
+    m_current_progress_up.reset(new Progress(
----------------
aprantl wrote:
> mib wrote:
> > I guess this could be refactored in a lambda function
> IIUC, this is redundant?
> Ah — unless this is about the ordering and having the old one destroyed first, in which case this makes sense.
yes it's ordering, destroying the previous event first, before constructing the new one. I will make the comment more clear.


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp:199
+    m_current_progress_up.reset(nullptr);
+    m_current_progress_up.reset(new Progress(
+        llvm::formatv("Currently building module {0}", module_name)));
----------------
aprantl wrote:
> For some reason I prefer writing
> `m_current_progress_up = std::make_unique<Progress>(...)`
> but I think for all practical intents this is equivalent.
that works for me, I thought about this too. This can also apply to the nullptr:

```
m_current_progress_up = nullptr;
```


================
Comment at: lldb/test/API/functionalities/progress_reporting/clang_modules/TestClangModuleBuildProgress.py:20
+        # Ensure an empty module cache.
+        mod_cache = os.path.join(self.getBuildDir(), "new-modules")
+        if os.path.isdir(mod_cache):
----------------
aprantl wrote:
> FYI this is the same as 
> `mod_cache = self.getBuildArtifact("new-modules")`
> 
cool


================
Comment at: lldb/test/API/functionalities/progress_reporting/clang_modules/TestClangModuleBuildProgress.py:36
+        # Trigger module builds.
+        self.expect("p @2")
+
----------------
aprantl wrote:
> This is a really indirect way of triggering a module import.
> Why not `expr -- @import Foo`?
> This way you can also import an empty module instead of an expensive uncached compilation of all of Foundation.
sounds great.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140056