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

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: source/Interpreter/CommandInterpreter.cpp:2708-2710
+    const char *data = str.data();
+    size_t size = str.size();
+    while (size > 0 && !WasInterrupted()) {
----------------
lemo wrote:
> lemo wrote:
> > lemo wrote:
> > > clayborg wrote:
> > > > Since we are using "llvm::StringRef" here, why not use its split functionality? Something like:
> > > > ```
> > > > bool done = false;
> > > > while (!done) {
> > > >   auto pair = str.split('\n');
> > > >   auto len = pair.first.size();
> > > >   done = pair.second.empty();
> > > >   // Include newline if we are not done
> > > >   if (!done)
> > > >     ++len; 
> > > >   stream.Write(pair.first.data(), 
> > > > }
> > > > ```
> > > > 
> > > > This approach also avoids the issue amccarth mentioned below about not ending with a newline. It is also quite a bit simpler to follow.
> > > I'll give it a try, thanks for the suggestion.
> > I forgot to reply to this: I tried using str.split() as suggested, with a small tweak to avoid reading past the end of the first slice (++len to include the '\n'):
> > 
> >     while (!str.empty() && !WasInterrupted()) {
> >       auto pair = str.split('\n');
> >       stream.Write(pair.first.data(), pair.first.size());
> >       if (str.size() != pair.first.size())
> >         stream.PutChar('\n');
> >       str = pair.second;
> >     }
> >     if (!str.empty()) {
> >       stream.Printf("\n... Interrupted.\n");
> >     }
> > 
> > While this version is shorter there I see a number of small problems with it:
> > 1. It requires a close look at the StringRef::split() interface (ex. done = pair.second.empty() is actually not the correct check since it will eat the final \n)
> > 2. While conceptually straightforward, the StringRef::split() interface is not ideal in this case - see #1
> > 3. This version assumes that stream.Write() actually writes everything (ie. not handling the return size)
> > 4. In order to avoid reading past the end of the first slice we need to do a separate PutChar() for each \n
> > 
> > So in the end I prefer the original version, which although a bit more verbose, tracks the invariants clearly, is potentially faster and handles strings regardless if they are terminated with \n or not.
> Thoughts?
I haven't followed the rest of this too closely, so forgive me if this is a dumb question, but it looks like by the time we reach this function, the command is already complete.  Why would you want to interrupt it if it's already done?  Couldn't you just print the entire output in one call to `stream.Write()` at this point?


https://reviews.llvm.org/D37923