kastiglione accepted this revision. kastiglione added inline comments. This revision is now accepted and ready to land.
================ Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4300-4301 + current_frame_flush)) + symbols_found = true; + flush |= current_frame_flush; + } ---------------- JDevlieghere wrote: > kastiglione wrote: > > JDevlieghere wrote: > > > kastiglione wrote: > > > > do you need the separate variable? can it be: > > > > > > > > ``` > > > > flush |= true; > > > > ``` > > > I want `flush` to be true only if at least one frame requires a flush. > > my suggestion still achieves that. > > > > Instead of: > > > > ``` > > bool current_frame_flush = false; > > if (DownloadObjectAndSymbolFile(module_spec, target, result, > > current_frame_flush)) > > symbols_found = true; > > > > flush |= current_frame_flush; > > ``` > > > > Inline the `current_frame_flush` variable: > > > > ``` > > if (DownloadObjectAndSymbolFile(module_spec, target, result, > > current_frame_flush)) > > flush |= true; > > ``` > > > > This makes `flush` only true when one frame needs it. > That relies on the assumption that flush is true iff > `DownloadObjectAndSymbolFile`return true (and the same for > `AddModuleSymbols`). Looking at the code that does indeed seem to be the > case, but then I'd rather see the API refactored to remove the `flush` output > variable instead of relying on an implementation-specific assumption here. thanks, makes sense. I had overlooked that `current_frame_flush` is also an output parameter. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110011/new/ https://reviews.llvm.org/D110011 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
