DavidSpickett added inline comments.
================ Comment at: lldb/test/API/driver/job_control/TestJobControl.py:9 +from lldbsuite.test.lldbtest import * +from lldbsuite.test import lldbutil +from lldbsuite.test.lldbpexpect import PExpectTest ---------------- Unused import? ================ Comment at: lldb/test/API/driver/job_control/TestJobControl.py:24 + self.launch(run_under=run_under, post_spawn=post_spawn) + child = self.child + ---------------- Am I correct that `self.launch` will wait until `post_spawn` finishes, meaning that self.child will always be set by this point? (or `post_spawn` timed out and we don't get here anyway) Also if the point is just to assert that self.child has been set, it would be a bit clearer to use an assert. Even if it is: ``` self.assertTrue(hasattr(self, "child")) ``` At least it looks like a deliberate check rather than a mistake if it fails. (I probably have the arguments the wrong way around, I always do somehow) ================ Comment at: lldb/test/API/driver/job_control/shell.py:10 + +def preexec_fn(): + orig_mask = signal.pthread_sigmask(signal.SIG_BLOCK, [signal.SIGTTOU]) ---------------- Please comment this function. I'm not sure if this intends to put the new process into a new process group or the same process group as this script. (but mostly because I haven't used process groups before now) ================ Comment at: lldb/tools/driver/Driver.cpp:834 signal(SIGTSTP, sigtstp_handler); - signal(SIGCONT, sigcont_handler); #endif ---------------- Is `sigcont_handler` now unused? It only references itself and is registered here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120320/new/ https://reviews.llvm.org/D120320 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
