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

[Jim Ingham via Phabricator via lldb-commits]

jingham marked an inline comment as done.
jingham added inline comments.


================
Comment at: lldb/source/Target/ThreadPlanStack.cpp:373
+  }
+}
----------------
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...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75880