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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked an inline comment as done.
JDevlieghere added inline comments.


================
Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4300-4301
+                                      current_frame_flush))
+        symbols_found = true;
+      flush |= current_frame_flush;
+    }
----------------
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. 


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

https://reviews.llvm.org/D110011