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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
