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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits