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

[Mike Aizatsky via llvm-commits]

aizatsky added a comment.

All done. PTAL.


================
Comment at: lib/sanitizer_common/sanitizer_common.h:283
@@ +282,3 @@
+// Starts a subprocess and returs its pid.
+//
+// If *_fd parameters are >=0 their corresponding input/output
----------------
samsonov wrote:
> But you *are* using "special values", harcoding the assumption that valid file descriptors are positive, and InvalidFd is negative.
> 
> This is not always so, e.g. on Windows fd_t is void*, so ">=0" check would always pass. It's better to use a named constant kInvalidFd than literal 0.
I didn't know that. Using kInvalidFd.



================
Comment at: lib/sanitizer_common/sanitizer_common.h:286
@@ +285,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, char *const argv[],
----------------
samsonov wrote:
> ... but you call `FileExists` in the implementation? So, e.g. this wouldn't work: StartSubprocess("llvm-symbolizer", argv, ....);
Thought about this. Removed FileExists. (I used it to debug things).

================
Comment at: lib/sanitizer_common/sanitizer_common.h:774
@@ +773,3 @@
+template <typename Fn>
+AutoRunner<Fn> at_scope_exit(Fn fn) {
+  return AutoRunner<Fn>(fn);
----------------
samsonov wrote:
> ... but you check for kInvalidFd three times in the implementation, though. Anyway, I don't have a strong opinion here.
Let's see how this one would be used. If we end up using it only for files - I'll create another utility.


http://reviews.llvm.org/D16546