[Lldb-commits] [PATCH] D110011: [lldb] Add --stack option to `target symbols add` command

[Dave Lee via Phabricator via lldb-commits]

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