[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:2713
+      for (; chunk_size < size; ++chunk_size) {
+        assert(data[chunk_size] != '\0');
+        if (data[chunk_size] == '\n') {
----------------
amccarth wrote:
> Should we be that trusting of a caller?  In a non-debug build, if a caller doesn't end the (non-empty) string with '\n', this'll just run past the end of the buffer.  Did I miss something that guarantees the caller won't make a mistake?  Would it be too expensive to play it safe?
There's no assumption that the string ends with \n, see the loop condition: chunk_size < size. The assert is just to validate that we don't have embedded NULs.


================
Comment at: source/Interpreter/CommandInterpreter.cpp:2708-2710
+    const char *data = str.data();
+    size_t size = str.size();
+    while (size > 0 && !WasInterrupted()) {
----------------
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.


================
Comment at: source/Interpreter/CommandInterpreter.cpp:2728
+  } else {
+    stream.PutCString(str);
+  }
----------------
clayborg wrote:
> llvm::StringRef can contain NULLs right? Maybe use
> 
> ```
> stream.Write(data, size);
> ```
The original code (line 2714) was using PutCString(), so this path is just preserving the original functionality (which also suggests that the output is not expected to have embedded nuls)


https://reviews.llvm.org/D37923