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

[Jim Ingham via Phabricator via lldb-commits]

jingham marked 2 inline comments as done.
jingham added a comment.

I added the enum, that's a good idea.

I agree that we should centralize internal reporting of Interrupt events, but as your "longer term" suggests, that's a design task and orthogonal to this patch.  I'll do that in a subsequent patch.



================
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);
----------------
mib wrote:
> 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.
I still don't see this.  GetFramesUpTo says clearly what the return value is, so I don't see why this is confusing.  You would only thing "why did GetFramesUpTo fail" if you didn't read the function definition.

Moreover, the code you are looking at will always do:

 if (GetFramesUpTo(idx, AllowInterruption)) {
    // Do something that's explicitly handling interruption
 }

so it should be pretty clear what is going on.

I don't think adding another parameter is warranted.  You would have to pass it even it you passed DoNotAllowInterruption, which is weird.  Or we could take a bool * but that's just more code to no very good purpose, IMO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150236