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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked 2 inline comments as done.
JDevlieghere added inline comments.


================
Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4267-4270
+    if (!StateIsStoppedState(process_state, true)) {
+      result.AppendErrorWithFormat("process is not stopped: %s",
+                                   StateAsCString(process_state));
+    }
----------------
kastiglione wrote:
> Is this missing a `return`?
Yep!


================
Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4280-4281
+    uint32_t frame_count = thread->GetStackFrameCount();
+    for (uint32_t i = 0; i < frame_count; ++i) {
+      lldb::StackFrameSP frame_sp = thread->GetStackFrameAtIndex(i);
+
----------------
kastiglione wrote:
> we don't have an iterator for this?
Not currently, no. I looked at `StackFrameList` in case it was just a matter of adding a `LockingAdaptedIterable`, but unsurprisingly,  stack frames are computed rather than stored in a list. We'd need to implement a custom iterator that keeps track of the index and calls `GetStackFrameAtIndex` under the hood, which is definitely something for a separate patch. 


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


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

https://reviews.llvm.org/D110011