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