[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via lldb-commits]

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 <jingham at 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 <zturner at 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 <zturner at 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 <jingham at 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 <
> reviews at 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
> > >
> > >
> > >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20170919/100acf7b/attachment-0001.html>