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
