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