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

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

This looks good to me, though I can't say I'm that familiar with thread plan intricacies.



================
Comment at: lldb/include/lldb/Target/ThreadPlanStack.h:118
+
+  void Update(ThreadList &current_threads);
+
----------------
Delete the dangling declaration as the current patch does not implement this.


================
Comment at: lldb/include/lldb/Target/ThreadPlanStack.h:126-130
+    auto result = m_plans_list.find(tid);
+    if (result == m_plans_list.end())
+      return false;
+    result->second.ThreadDestroyed(nullptr);
+    m_plans_list.erase(result);
----------------
Any chance of calling `ThreadDestroyed` from ThreadPlanStack destructor, so this can just be `m_plan_list.erase(tid)` ?


================
Comment at: lldb/source/Target/ThreadPlanStack.cpp:373
+  }
+}
----------------
jingham wrote:
> labath wrote:
> > 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.
> The "may" in that sentence is apparently the crucial bit, but it didn't explain why an enum specified only by named constants might have other possible values.  I read a little further on, but didn't get to the part where it explains that.  Seems to me in this case the compiler shouldn't have to to guess about the extent of the valid values of this enum and then be conservative or not in handling the results of its guess.  I don't see the point of an enum if a legally constructed enum value (not cast from some random value) isn't exhausted by the constants used to define it.  That just seems weird.
> 
> But arguing with C++ compiler diagnostics isn't a productive use of time, and I feel like too deep an understanding of the C++ type system's mysteries is not good for one's mental stability.  So I'm more than content to cargo cult this one...
The rough rule is that for enums without a fixed underlying type, the range of valid values is those that are representable by the smallest bitfield capable of holding all enumerators of that enum. For a fixed underlying type, the range is the whole type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75880