[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:
> 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?


https://reviews.llvm.org/D37923