[PATCH] D47913: [Support] Introduce a new utility for testing child process execution

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

I like the idea. Some comments inline.



================
Comment at: llvm/utils/trace/Trace.cpp:19-20
+
+#include <io.h>
+
+using namespace llvm;
----------------
Is `io.h` really needed?


================
Comment at: llvm/utils/trace/Trace.cpp:92-96
+  } else {
+    errs() << formatv(
+        "CHILD(stderr): Invalid argument '{0}' to command 'read'\n", Source);
+    exit(-1);
+  }
----------------
Could probably factor this out in a report_error function as the other tools do (or just call report_fatal_error if you can).


================
Comment at: llvm/utils/trace/Trace.cpp:148-149
+    outs().flush();
+    int ExitCode = run();
+    return ExitCode;
+  }
----------------
`return run()` ?


================
Comment at: llvm/utils/trace/Trace.cpp:152
+
+  std::array<llvm::Optional<StringRef>, 3> Redirects;
+
----------------
`SmallVector` ?


================
Comment at: llvm/utils/trace/Trace.cpp:167-168
+  int Result = sys::ExecuteAndWait(argv[0], Args, nullptr, Redirects);
+  outs() << "PARENT(stdout): Child exited with code " << Result << "\n";
+  outs().flush();
+  return 0;
----------------
As we're basically flushing everytime we write to `outs()` should we provide a wrapper for this?


https://reviews.llvm.org/D47913