[Lldb-commits] [PATCH] D119388: [lldb/Plugin] Add artificial stackframe loading in ScriptedThread

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:159
+
+  size_t arr_size = arr_sp->GetSize();
+  if (arr_size > std::numeric_limits<uint32_t>::max())
----------------
`const`


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:195
+
+    lldb::addr_t cfa = LLDB_INVALID_ADDRESS;
+    bool cfa_is_valid = false;
----------------
`const`


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:196
+    lldb::addr_t cfa = LLDB_INVALID_ADDRESS;
+    bool cfa_is_valid = false;
+    const bool behaves_like_zeroth_frame = false;
----------------
`const`


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:273
   GetRegisterContext()->InvalidateIfNeeded(/*force=*/false);
+  LoadArtificialStackFrames();
 }
----------------
mib wrote:
> jingham wrote:
> > LoadArtificialStackFrames returns a bool for success/failure.  It always bugs me when returns signifying errors get dropped on the floor like this.
> `Thread::RefreshStateAfterStop` doesn't return anything, and the failure case is already handled in `LoadArtificialStackFrames` by logging an error message in the appropriate channel.
> 
> Do you have any other suggestion on how I could improve this ?
If we are never going to check the return value, we should not have one, it is meaningless.


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

https://reviews.llvm.org/D119388