[Lldb-commits] [PATCH] D150236: Thread::GetStackFrameCount should forestall interruption

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.


================
Comment at: lldb/include/lldb/Target/StackFrameList.h:106
+  /// Returns true if the function was interrupted, false otherwise.
+  bool GetFramesUpTo(uint32_t end_idx, bool allow_interrupt = true);
 
----------------
JDevlieghere wrote:
> I personally would prefer to have an `InterruptPolicy`  (e.g. `AllowInterrupt`, `DenyInterrupt`) to limit the proliferation of boolean flags and improve readability. 
+1, this would greatly improve readability


================
Comment at: lldb/include/lldb/Target/StackFrameList.h:103-104
 
-  void GetFramesUpTo(uint32_t end_idx);
+  /// Gets frames up to end_idx (which can be greater than the actual number of
+  /// frames.)  Returns true if the function was interrupted, false otherwise.
+  bool GetFramesUpTo(uint32_t end_idx, bool allow_interrupt = true);
----------------
jingham wrote:
> mib wrote:
> > bulbazord wrote:
> > > Is the range inclusive of `end_idx`? as in, is it [0, end_idx) or [0, end_idx]?
> > @bulbazord I believe this should be inclusive.
> > 
> > @jingham This comment sounds like we only return `true` if the function was interrupted, which is not the expected behavior, right ?
> The function used to return void, so it had no expected behavior before.  I am defining the "bool" return here, and I define it to be "was_interrupted" as I say in the doc string.  That's also why the return values you are questioning below are actually correct.  This is NOT returning "success" it is returning "interrupted".
Given that this changes the behavior of `GetFramesUpTo`, I'd rather pass a bool reference that says `is_interrupted` and keep `void` as the return type, because `if (GetFramesUpTo(idx, true) == false)` really doesn't tell me much. Looking at this code, I'd think why did `GetFramesUpTo` failed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150236