[Lldb-commits] [PATCH] D75880: [NFC} Move ThreadPlans stacks into their own class, store it in Process by TID

Wed Mar 11 04:35:15 PDT 2020

labath added a comment.

In D75880#1915999 <https://reviews.llvm.org/D75880#1915999>, @jingham wrote:

>   We use that all over the place in lldb, and I much prefer it. 

I'm afraid that's no longer true. I count 30 instances of `&&` or `||` at the beginning of a line versus 2343 instances of it being at the end. One of the main advantages of having a tool like clang-format is that you don't have to argue about the right way to format something. It may not produce the nicest possible layout all the time, but it will be close enough most of the time. And it will be consistent...

Overall, I'd just recommend getting into the habit of running clang-format before submitting a patch. I noticed a bunch of inconsistencies in the formatting of this patch. Right now, I only highlighted those that are definitely inferior to what clang-format would produce.

================
Comment at: lldb/include/lldb/Target/ThreadPlanStack.h:32
+class ThreadPlanStack {
+friend class lldb_private::Thread;
+
----------------
this should be indented

================
Comment at: lldb/include/lldb/Target/ThreadPlanStack.h:105-108
+ const PlanStack &GetStackOfKind(ThreadPlanStack::StackKind kind) const;
+ 
+ PlanStack m_plans; ///< The stack of plans this thread is executing.
+ PlanStack m_completed_plans; ///< Plans that have been completed by this
----------------
inconsistent indentation

================
Comment at: lldb/include/lldb/Target/ThreadPlanStack.h:156-157
+private:
+    using PlansList = std::unordered_map<lldb::tid_t, ThreadPlanStack>;
+    PlansList m_plans_list;
+};
----------------
here too

================
Comment at: lldb/source/Target/Process.cpp:1261
+    return llvm::make_error<llvm::StringError>(
+        llvm::formatv("Invalid option with value '{0}'.", tid),
+        llvm::inconvertibleErrorCode());
----------------
jingham wrote:
> labath wrote:
> > 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?
> I originally had this as a pointer that returns NULL when not found, but I thought we were going towards Expected's for everything that might not produce a value.  I'm happier with it as just a pointer, so I put it back to that.
I don't think we want to use Expected everywhere. We definitely trying to avoid returning default-constructed objects and checking them with IsValid or something. And if I am creating a new class, I try to make it so that it does not have an "invalid" state and use Optionals or Expecteds to signal its absence. But pointers are existing type with a well known "invalid" value, so I don't see a problem with using it. Furthermore, with pointers it is possible to express that something is always valid (by using a reference), which is something that isn't doable with a class with an IsValid method.

Expecteds are mostly useful when you actually want to return an error from a function (instead of a Status + default value combo), for instance when there are multiple ways it can fail, and you want to user to know what happened. Or if you think that it is dangerous to ignore possible errors from the function and you want to make sure the user checks for them. For simple getter-like functions, I don't think any of this applies.

================
Comment at: lldb/source/Target/Thread.cpp:1059-1060
+void Thread::PushPlan(ThreadPlanSP thread_plan_sp) {
+  if (!thread_plan_sp)
+    assert("Don't push an empty thread plan.");

----------------
I meant `assert(thread_plan_sp && "Don't push an empty thread plan.")`

================
Comment at: lldb/source/Target/ThreadPlanStack.cpp:233-234
+lldb::ThreadPlanSP ThreadPlanStack::GetCurrentPlan() const {
+  if (m_plans.size() == 0)
+    llvm_unreachable("There will always be a base plan.");
+  return m_plans.back();
----------------
assert(!empty)

================
Comment at: lldb/source/Target/ThreadPlanStack.cpp:373
+  }
+}
----------------
jingham wrote:
> labath wrote:
> > 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.
> I added a default, but what do you mean by "if we are to be fully pedantic, it isn't"?  It's an enum with 3 cases, all of which are listed.  How is that not fully covered?
The first sentence of [[ https://en.cppreference.com/w/cpp/language/enum | cppreference enum definition ]] could be helpful here:
```
An enumeration is a distinct type whose value is restricted to a range of values (see below for details), which may include several explicitly named constants ("enumerators").
```
What exactly is the "range of values" of an enum is a complex topic, but suffice to say it usually includes more than the named constants one has mentioned in the enum definition (imagine bitfield enums).
Clang takes the "common sense" approach and assumes that if one does a switch over an enum, he expects it to always have one of the named constants. Gcc follows a stricter interpretation and concludes that there are execution flows which do not return a value from this function and issues a warning. Which behavior is more correct is debatable...

Unfortunately adding a default case is not the right solution here (though I can see how that could be understood from my comment -- sorry), because clang will then warn you about putting a default statement in what it considers to be a fully covered switch. The right solution is to put the llvm_unreachable call *after* the switch statement, which is the layout that will make everyone happy. You don't need (or shouldn't) put the return statement because the compilers know that the macro terminates the progream.

I'm sorry that this turned into such a lesson, but since you were using the macro in an atypical way, I wanted to illustrate what is the more common use of it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75880