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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: source/Commands/CommandObjectTarget.cpp:2056-2058
+            if (m_interpreter.WasInterrupted()) {
+              break;
+            }
----------------
Remove braces


================
Comment at: source/Commands/CommandObjectTarget.cpp:2087-2089
+                if (m_interpreter.WasInterrupted()) {
+                  break;
+                }
----------------
Remove braces


================
Comment at: source/Commands/CommandObjectTarget.cpp:2155-2157
+            if (m_interpreter.WasInterrupted()) {
+              break;
+            }
----------------
Remove braces


================
Comment at: source/Commands/CommandObjectTarget.cpp:2179-2181
+              if (m_interpreter.WasInterrupted()) {
+                break;
+              }
----------------
Remove braces


================
Comment at: source/Commands/CommandObjectTarget.cpp:2254-2256
+            if (m_interpreter.WasInterrupted()) {
+              break;
+            }
----------------
Remove braces


================
Comment at: source/Commands/CommandObjectTarget.cpp:2278-2280
+              if (m_interpreter.WasInterrupted()) {
+                break;
+              }
----------------
Remove braces


================
Comment at: source/Commands/CommandObjectTarget.cpp:2348-2350
+            if (m_interpreter.WasInterrupted()) {
+              break;
+            }
----------------
Remove braces


================
Comment at: source/Interpreter/CommandInterpreter.cpp:2682
+  auto prev_state = m_command_state.exchange(CommandHandlingState::eInProgress);
+  assert(prev_state == CommandHandlingState::eIdle);
+}
----------------
lldb_assert


================
Comment at: source/Interpreter/CommandInterpreter.cpp:2687
+  auto prev_state = m_command_state.exchange(CommandHandlingState::eIdle);
+  assert(prev_state != CommandHandlingState::eIdle);
+}
----------------
lldb_assert


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


================
Comment at: source/Interpreter/CommandInterpreter.cpp:2728
+  } else {
+    stream.PutCString(str);
+  }
----------------
llvm::StringRef can contain NULLs right? Maybe use

```
stream.Write(data, size);
```


https://reviews.llvm.org/D37923