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

Adrian McCarthy via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Fri Sep 15 13:48:37 PDT 2017

amccarth added inline comments.

Comment at: source/Commands/CommandObjectTarget.cpp:2058
+              break;
+            }
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()) {
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.


More information about the lldb-commits mailing list