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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Nice! I have a suggestion to speed up the test and make it a little more robust.



================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp:82
   Log *m_log;
+  std::unique_ptr<Progress> m_current_progress_up;
+  std::vector<std::string> m_module_build_stack;
----------------
kastiglione wrote:
> mib wrote:
> > `Progress` makes use of RAII to complete the progress report event.
> > 
> > I think a local variable would be more suitable.
> Since this is a callback, and the work being done doesn't happen in this function, I reasoned that that a local wouldn't match the semantics. Since this function is so short lived, the Progress would be immediately destructed and completed, but the work of building the module is not yet completed. Also, when an event is completed, the console text is cleared. As a result, the user doesn't see which module is currently building.
Might be good to comment this?


================
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(
----------------
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.


================
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)));
----------------
For some reason I prefer writing
`m_current_progress_up = std::make_unique<Progress>(...)`
but I think for all practical intents this is equivalent.


================
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):
----------------
FYI this is the same as 
`mod_cache = self.getBuildArtifact("new-modules")`



================
Comment at: lldb/test/API/functionalities/progress_reporting/clang_modules/TestClangModuleBuildProgress.py:36
+        # Trigger module builds.
+        self.expect("p @2")
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140056