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

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added inline comments.


================
Comment at: source/Commands/CommandObjectTarget.cpp:2058
+              break;
+            }
             num_dumped++;
----------------
amccarth wrote:
> Many LLVM developers prefer to omit the braces when the body of the control-flow statement is a single statement.
So do the hackers: https://blog.codecentric.de/en/2014/02/curly-braces :) I too prefer to omit braces in small test snippets, but in production code it's not worth the risk of making a silly mistake.


================
Comment at: source/Interpreter/CommandInterpreter.cpp:2720
+      chunk_size = stream.Write(data, chunk_size);
+      assert(size >= chunk_size);
+      data += chunk_size;
----------------
amccarth wrote:
> This assert should precede the line before it.
Pedantically, it should be both before and after (and for ultimate paranoid mode, asserting that Write returns <= than the passed in value)

But the asserts looks for the really nasty case where "size -= chunk_size" overflows.


================
Comment at: tools/driver/Driver.cpp:1189
 
-  exit(signo);
+  _exit(signo);
 }
----------------
amccarth wrote:
> Can you add a comment explaining why this uses `_exit` rather than `exit`?  It's not obvious to me.
Explained in the SIGINT patch: exit() is not signal-safe (http://pubs.opengroup.org/onlinepubs/000095399/functions/exit.html)

Calling it from a signal handler can result in all kind of nasty issues, in particular exit() does call a lot of stuff, both runtime and user code (ex. atexit functions) 



================
Comment at: tools/lldb-mi/MIDriverMain.cpp:71
 //--
 void sigint_handler(int vSigno) {
 #ifdef _WIN32 // Restore handler as it is not persistent on Windows
----------------
amccarth wrote:
> 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.
Agreed, I already split this change into separate patches (I wasn't sure if people prefer to review two small changes vs a single one with more context)


https://reviews.llvm.org/D37923