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

[Leonard Mosescu via Phabricator via lldb-commits]

lemo 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:
> 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.


https://reviews.llvm.org/D37923