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