labath added a comment.
As we've seen, I don't know much about thread plans so I don't have much to say
on the higher-level aspects of this patch.
But I see a bunch of opportunities to make this more consistent with llvm code
style.
================
Comment at: lldb/include/lldb/Target/ThreadPlanStack.h:9
+
+#ifndef liblldb_ThreadPlanStack_h_
+#define liblldb_ThreadPlanStack_h_
----------------
the include guards were recently normalized to LLDB_TARGET_THREADPLANSTACK_H
================
Comment at: lldb/include/lldb/Target/ThreadPlanStack.h:35
+public:
+ ThreadPlanStack(Thread &thread){};
+ ~ThreadPlanStack() {}
----------------
superfluous semicolon
================
Comment at: lldb/include/lldb/Target/ThreadPlanStack.h:113
+
+class ThreadPlanStackMap {
+public:
----------------
It's not clear to me what is the value of this class. Most of it is just about
wrapping the underlying container in a slightly different way. If the
ThreadDestroyed method can be called in the ThreadPlanStack destructor (which
appears to be possible and would be a nice clean up regardless) then RemoveTID
would boil down to `container.erase(tid)`. At that point the only non-trivial
piece of functionality is the `Update` function (which this patch doesn't even
implement), which could just as well be implemented outside of the class.
================
Comment at: lldb/source/Target/Process.cpp:1261
+ return llvm::make_error<llvm::StringError>(
+ llvm::formatv("Invalid option with value '{0}'.", tid),
+ llvm::inconvertibleErrorCode());
----------------
This error message doesn't really make sense for a function like this. What
option is it talking about?
As the only reason this can fail is because there are no plans for the given
tid, maybe you don't actually need the Expected, and can signal that with a
simple nullptr?
================
Comment at: lldb/source/Target/Thread.cpp:505
saved_state.current_inlined_depth = GetCurrentInlinedDepth();
- saved_state.m_completed_plan_stack = m_completed_plan_stack;
+ saved_state.m_completed_plan_checkpoint =
+ GetPlans().CheckpointCompletedPlans();
----------------
What's the reason for the introduction of the "checkpoint" indirection? Does it
have something to do with needing to update the checkpointed plans when the
thread disappears?
================
Comment at: lldb/source/Target/Thread.cpp:1054-1058
+ llvm::Expected<ThreadPlanStack *> plans =
+ GetProcess()->FindThreadPlans(GetID());
+ if (!plans)
+ llvm_unreachable("Thread left with an empty plan stack");
+ return **plans;
----------------
maybe `return cantFail(GetProcess()->FindThreadPlans(GetID()));`
================
Comment at: lldb/source/Target/Thread.cpp:1061
+
void Thread::PushPlan(ThreadPlanSP &thread_plan_sp) {
+ if (!thread_plan_sp)
----------------
You don't modify the callers shared pointer => `const ThreadPlanSP &` or a
`ThreadPlanSP` with a PushPlan(std::move(thread_plan_sp))`.
================
Comment at: lldb/source/Target/Thread.cpp:1062-1063
void Thread::PushPlan(ThreadPlanSP &thread_plan_sp) {
- if (thread_plan_sp) {
- // If the thread plan doesn't already have a tracer, give it its parent's
- // tracer:
- if (!thread_plan_sp->GetThreadPlanTracer()) {
- assert(!m_plan_stack.empty());
- thread_plan_sp->SetThreadPlanTracer(
- m_plan_stack.back()->GetThreadPlanTracer());
- }
- m_plan_stack.push_back(thread_plan_sp);
+ if (!thread_plan_sp)
+ llvm_unreachable("Don't push an empty thread plan.");
----------------
assert
================
Comment at: lldb/source/Target/ThreadPlanStack.cpp:72-73
+ auto result = m_completed_plan_store.find(checkpoint);
+ if (result == m_completed_plan_store.end())
+ llvm_unreachable("Asked for a checkpoint that didn't exist");
+ m_completed_plans.swap((*result).second);
----------------
The usual way to write that is `assert(result == m_completed_plan_store.end()
&& "Asked for a checkpoint that didn't exist")`. llvm_unreachable is used where
an existing control flow path needs to be declared invalid. We don't introduce
new control flow just for the sake of using it.
================
Comment at: lldb/source/Target/ThreadPlanStack.cpp:84-90
+ for (auto plan : m_plans)
+ plan->ThreadDestroyed();
+
+ for (auto plan : m_discarded_plans)
+ plan->ThreadDestroyed();
+
+ for (auto plan : m_completed_plans)
----------------
Please put the actual type (ThreadPlanSP ?) instead of `auto`.
================
Comment at: lldb/source/Target/ThreadPlanStack.cpp:126-127
+ // The first plan has to be a base plan:
+ if (m_plans.size() == 0 && !new_plan_sp->IsBasePlan())
+ llvm_unreachable("Zeroth plan must be a base plan");
+
----------------
assert instead of branch-to-unreachable
================
Comment at: lldb/source/Target/ThreadPlanStack.cpp:373
+ }
+}
----------------
This is the usual place where one would add a llvm_unreachable. Some compilers
do not consider the above switch to be fully covered (and if we are to be fully
pedantic, it isn't) and will complain about falling off of the end of a
non-void function.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75880/new/
https://reviews.llvm.org/D75880
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
