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