[PATCH] D16546: [sanitizers] extracted process management functions

[Filipe Cabecinhas via llvm-commits]

filcab added a subscriber: filcab.
filcab added a comment.

Thanks for working on this.


================
Comment at: lib/sanitizer_common/sanitizer_common.h:284
@@ +283,3 @@
+// If *_fd parameters are not kInvalidFd their corresponding input/output
+// streams would be redirect to the file. The files would always be closed
+// in parent process even in case of an error.
----------------
Nit: s/would/will/g

================
Comment at: lib/sanitizer_common/sanitizer_common.h:285
@@ +284,3 @@
+// streams would be redirect to the file. The files would always be closed
+// in parent process even in case of an error.
+int StartSubprocess(const char *filename, const char *const argv[],
----------------
We should probably add that all fd after STDERR_FILENO get closed.

================
Comment at: lib/sanitizer_common/sanitizer_common.h:290
@@ -282,1 +289,3 @@
+// Checks if specified process is still running
+bool IsProcessRunning(int pid);
 
----------------
Should this argument (and StartSubprocess' return value) be something else, since, as Alexey said, "on Windows fd_t is void*"?
Probably either a typedef or just a uptr?

================
Comment at: lib/sanitizer_common/sanitizer_common.h:761
@@ +760,3 @@
+template <typename Fn>
+class AutoRunner {
+ public:
----------------
I'd prefer a more descriptive name (`RunOnDestruction` or something), even if it's longer. It seems it's mostly used in utility functions (`at_scope_exit`), which also makes a longer name a smaller problem.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:142
@@ +141,3 @@
+    GetArgV(path_, argv);
+    pid = StartSubprocess(path_, argv, outfd[0] /* stdin */,
+                          infd[1] /* stdout */);
----------------
Nit: I think we usually have the comment before the parameter.


http://reviews.llvm.org/D16546