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

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added inline comments.


================
Comment at: source/Commands/CommandObjectTarget.cpp:2058
+              break;
+            }
             num_dumped++;
----------------
Many LLVM developers prefer to omit the braces when the body of the control-flow statement is a single statement.


================
Comment at: source/Interpreter/CommandInterpreter.cpp:2713
+      for (; chunk_size < size; ++chunk_size) {
+        assert(data[chunk_size] != '\0');
+        if (data[chunk_size] == '\n') {
----------------
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?


================
Comment at: source/Interpreter/CommandInterpreter.cpp:2720
+      chunk_size = stream.Write(data, chunk_size);
+      assert(size >= chunk_size);
+      data += chunk_size;
----------------
This assert should precede the line before it.


================
Comment at: tools/driver/Driver.cpp:1182
   if (g_driver) {
-    if (!g_interrupt_sent) {
-      g_interrupt_sent = true;
+    if (!g_interrupt_sent.test_and_set()) {
       g_driver->GetDebugger().DispatchInputInterrupt();
----------------
I'm not sure why these two ifs aren't one, as in:

```
if (g_driver && !g_interrupt_sent.test_and_set())
```


================
Comment at: tools/driver/Driver.cpp:1189
 
-  exit(signo);
+  _exit(signo);
 }
----------------
Can you add a comment explaining why this uses `_exit` rather than `exit`?  It's not obvious to me.


================
Comment at: tools/lldb-mi/MIDriverMain.cpp:71
 //--
 void sigint_handler(int vSigno) {
 #ifdef _WIN32 // Restore handler as it is not persistent on Windows
----------------
I think this concurrency fix for SIGINT would be better in a separate patch.  I understand how it's related to the rest of this patch, but LLVM folks tend to prefer small, incremental patches.


https://reviews.llvm.org/D37923