That seems fine to me. Jim
> On Sep 19, 2017, at 11:39 AM, Leonard Mosescu <mose...@google.com> wrote: > > So, how about I look into exposing WasInterrupted() through SB APIs in a > follow up change? > > On Tue, Sep 19, 2017 at 11:36 AM, Jim Ingham <jing...@apple.com> wrote: > Xcode does, I don't know about other UI's. > > Jim > > > On Sep 19, 2017, at 11:35 AM, Leonard Mosescu <mose...@google.com> wrote: > > > > I agree Jim. I'd like like to build thing incrementally - checking in the > > current change as is does not preclude adding the SB APIs while it does > > provide the foundation. > > > > I think that going after the scenarios you mention is a significant > > increase in scope. Are people even hooking up DispatchInterrupt() from > > whatever interactive driver they use? > > > > On Tue, Sep 19, 2017 at 11:27 AM, Jim Ingham <jing...@apple.com> wrote: > > We agreed to forwards compatibility because people write big scripts that > > use the SB API, implement GUI's on top of them (more than just Xcode) etc. > > So we try not to jerk those folks around. That adds a little more > > responsibility on our part to think carefully about what we add, but the > > notion that we should refrain from making useful functionality available > > because we'd rather not be beholden to our decisions seems really > > wrong-headed to me. > > > > And in this case there's a clear use for this. For instance the xnu macros > > have a bunch of Python based commands that spew out pages and pages of > > output. Those guys would love to make their commands interruptible. To do > > that they would need to call WasInterrupted. So this is 100% something > > that should be available at the SB API layer. > > > > Jim > > > > > > > > > On Sep 19, 2017, at 11:18 AM, Zachary Turner <ztur...@google.com> wrote: > > > > > > Also, it avoids polluting the SB interface with another function that > > > probably nobody is ever going to use outside of testing. Adding to the > > > SB API should take an act of God, given that once it gets added it has to > > > stay until the end of time. > > > > > > On Tue, Sep 19, 2017 at 11:15 AM Zachary Turner <ztur...@google.com> > > > wrote: > > > That would work, but I think it's adding many more pieces to the test. > > > Now there's threads, a Python layer, and multiprocess dotest > > > infrastructure in the equation. Each providing an additional source of > > > flakiness and instability. > > > > > > If all it is is a pass-through, then just calling the function it passes > > > through to is a strictly better test, since it's focusing the test on the > > > underlying piece of functionality and removing additional sources of > > > failure and instability from the test. > > > > > > On Tue, Sep 19, 2017 at 11:02 AM Jim Ingham <jing...@apple.com> wrote: > > > I'd really prefer to do this as/along with an SB API test since we also > > > need commands made through the SB API to be interruptible, and we should > > > test that that also works. So this kills two birds with one stone. > > > > > > In general, when developing any high-level features in lldb one of the > > > questions you should ask yourself is whether this is useful at the SB API > > > layer. In this case it obviously is (actually I'm a little embarrassed I > > > didn't think of this requirement). If so, then it's simplest to test it > > > at that layer. If the SB API is anything more than a pass-through, then > > > you might also want to write a unit test for the underlying C++ API's. > > > But in this case the SB function will just call the underlying > > > CommandInterpreter one, so testing at the SB layer is sufficient. > > > > > > Jim > > > > > > > On Sep 19, 2017, at 10:45 AM, Zachary Turner via Phabricator > > > > <revi...@reviews.llvm.org> wrote: > > > > > > > > zturner added a comment. > > > > > > > > In https://reviews.llvm.org/D37923#875322, @clayborg wrote: > > > > > > > >> We should have a test. The test would need to use pexpect or something > > > >> similar. If anyone has any pointer on how to make a test for this, > > > >> please chime in. I was thinking just a pexpect based test. > > > > > > > > > > > > This kind of thing is perfect for a unit test, but I'm not sure how > > > > easy that would be with the current design. You'd basically do > > > > something like: > > > > > > > > struct MockStream { > > > > explicit MockStream(CommandInterpreter &Interpreter, int > > > > InterruptAfter) > > > > : CommandInterpreter(Interpreter), InterruptAfter(InterruptAfter) > > > > {} > > > > > > > > CommandInterpreter &Interpreter; > > > > const int InterruptAfter; > > > > int Lines = 0; > > > > std::string Buffer; > > > > > > > > void Write(StringRef S) { > > > > ++Lines; > > > > if (Lines >= InterruptAfter) { > > > > Interpreter.Interrupt(); > > > > return; > > > > } > > > > Buffer += S; > > > > } > > > > }; > > > > > > > > TEST_F(CommandInterruption) { > > > > CommandInterpreter Interpreter; > > > > MockStream Stream(Interpreter, 3); > > > > Interpreter.PrintCommandOutput(Stream, "a\nb\nc\nd\ne\nf\n"); > > > > EXPECT_EQ(Stream.Lines == 3); > > > > EXPECT_EQ(Stream.Buffer == "a\nb\nc\n"); > > > > } > > > > > > > > > > > > https://reviews.llvm.org/D37923 > > > > > > > > > > > > > > > > > > > > > _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
