[Lldb-commits] [PATCH] D147753: Move "SelectMostRelevantFrame" from Thread::WillStop

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.


================
Comment at: lldb/include/lldb/Target/StackFrameList.h:139-144
   /// The currently selected frame.
   uint32_t m_selected_frame_idx;
+  // Records whether anyone has set the selected frame on this stack yet.
+  // We only let recognizers change the frame if this is the first time
+  // GetSelectedFrame is called.
+  bool m_selected_frame_set = false;
----------------
JDevlieghere wrote:
> The purpose of the boolean is to differentiate between `m_selected_frame_idx == 0` meaning the zeroth frame is selected versus we haven't yet selected the current frame, right? If so I think we should make this an `std::optional<bool>`.
I don't follow.  Did you mean make `m_selected_frame_idx` a `std::optional<uint32_t>`?  

You could do it that way for sure.  I didn't because I'm not as fond of gadgets.  Most clients should be calling GetSelectedFrameIndex which will always set it to something (0 if it can't figure anything else out).  So the optionality shouldn't be much visible.


================
Comment at: lldb/source/Target/StackFrameList.cpp:823
+    SelectMostRelevantFrame();
+    m_selected_frame_set = true;
+  }  
----------------
JDevlieghere wrote:
> IIUC `SetSelectedFrame` always sets `m_selected_frame_set` to `true`, so is this redundant?
 Sure, but if you do that you have to change SelectMostRelevantFrame to select the 0th frame if there's no relevant frame.  At present it leaves it unselected.  Doing the selection always in SMRF  is clearer, though, so it would be a good change.


================
Comment at: lldb/source/Target/StackFrameList.cpp:883
   m_concrete_frames_fetched = 0;
+  m_selected_frame_set = false;
 }
----------------
JDevlieghere wrote:
> Should `m_selected_frame_idx` also be set to `0` here?
Yes, though this function doesn't currently matter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147753