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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
