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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
