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