[PATCH] D37563: Convenience/safety fix for llvm::sys::Execute(And|No)Wait

[Alexander Kornienko via Phabricator via llvm-commits]

alexfh added inline comments.


================
Comment at: tools/llvm-cov/CodeCoverage.cpp:460
   ArgsV.push_back(nullptr);
-  StringRef InputPathRef = InputPath.str();
-  StringRef OutputPathRef = OutputPath.str();
-  StringRef StderrRef;
-  const StringRef *Redirects[] = {&InputPathRef, &OutputPathRef, &StderrRef};
+  Optional<StringRef> Redirects[] = {InputPath.str(), OutputPath.str(), {""}};
   std::string ErrMsg;
----------------
vsk wrote:
> vsk wrote:
> > You've used llvm::None for the stderr redirect, and I think it would be clearer / more consistent to use llvm::None here too.
> Sorry, I meant to write: 'you've used None stderr redirect earlier in lib/Support/Signals.cpp'. I'm not sure what the difference is here.
The difference is that `None` would disable redirection of the corresponding file descriptor (so stderr would be output to the parent's stderr) and an empty string redirects the file descriptor to nowhere (so stderr would be just dropped). This behavior is the same as before the patch (a `nullptr` was used instead of `llvm::None`). Here the original code wanted a redirect to nowhere, which the new code does as well.


https://reviews.llvm.org/D37563