[Lldb-commits] [PATCH] D114862: Replace StackID's operator "<" with a function returning FrameComparison

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.
Herald added a project: All.

IIUC, the crux of this change is having the comparison able to return eFrameCompareUnknown when the `<` operator was returning "less than", and the handling of Unknown in the various thread plans.  Is that right?  There's so much formal change here it's hard to see what you are actually doing.
So the functional parts of the change are just the couple of places where you added eFrameCompareUnknown handling that does an additional "pc" compare.  Is that right?  If so, that seems fine to me, though it would be much nicer if there were examples we could use to test this new behavior.

I actually like having the CompareTo rather than doing a bunch if if ==, then else if < then else.  So that part seems good.  But it's a little hard to grok the extent of this change.



================
Comment at: lldb/source/Target/StackID.cpp:33
 
-bool lldb_private::operator==(const StackID &lhs, const StackID &rhs) {
-  if (lhs.GetCallFrameAddress() != rhs.GetCallFrameAddress())
-    return false;
-
-  SymbolContextScope *lhs_scope = lhs.GetSymbolContextScope();
-  SymbolContextScope *rhs_scope = rhs.GetSymbolContextScope();
-
-  // Only compare the PC values if both symbol context scopes are nullptr
-  if (lhs_scope == nullptr && rhs_scope == nullptr)
-    return lhs.GetPC() == rhs.GetPC();
-
-  return lhs_scope == rhs_scope;
-}
-
-bool lldb_private::operator!=(const StackID &lhs, const StackID &rhs) {
-  if (lhs.GetCallFrameAddress() != rhs.GetCallFrameAddress())
-    return true;
-
-  SymbolContextScope *lhs_scope = lhs.GetSymbolContextScope();
-  SymbolContextScope *rhs_scope = rhs.GetSymbolContextScope();
-
-  if (lhs_scope == nullptr && rhs_scope == nullptr)
-    return lhs.GetPC() != rhs.GetPC();
+lldb::FrameComparison StackID::CompareTo(const StackID &rhs) const {
+  if (*this == rhs)
----------------
It would be easier to read this if the operator== were above CompareTo rather than below it.  The first thing this function does is call the == comparator, so it would be nice to have that right next to where it's called.  

It's seems a little odd at first to still have the == operator, you'd thing CompareTo should handle everything and then the == should just be `lhs.CompareTo(rhs) == eFrameCompareEqual` but the CompareTo computation does work that's not needed for ==, so actually that part is fine.


================
Comment at: lldb/source/Target/ThreadPlanStepOut.cpp:513
+
+bool ThreadPlanStepOut::DoesCurrentFrameExplainStop() {
+  StackID frame_zero_id = GetThread().GetStackFrameAtIndex(0)->GetStackID();
----------------
"Current" is confusing, since in the context of frames we use it as part of "Currently Selected Frame" most often, which this is not treating.  This has also become a somewhat obscure function, so it definitely needs some comment saying why this computation does what the function name says it does.



================
Comment at: lldb/source/Target/ThreadPlanStepRange.cpp:219
 
-  if (cur_frame_id == m_stack_id) {
-    frame_order = eFrameCompareEqual;
-  } else if (cur_frame_id < m_stack_id) {
-    frame_order = eFrameCompareYounger;
-  } else {
-    StackFrameSP cur_parent_frame = thread.GetStackFrameAtIndex(1);
-    StackID cur_parent_id;
-    if (cur_parent_frame)
-      cur_parent_id = cur_parent_frame->GetStackID();
+  auto frame_order = cur_frame_id.CompareTo(m_stack_id);
+  if (frame_order != eFrameCompareUnknown)
----------------
This doesn't seem like a good use of auto. In an IDE it's nice to be able to do "Jump to definition" to see the options here, which is harder to do with auto than with the actual type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114862