This is an archive of the discontinued LLVM Phabricator instance.

Convenience/safety fix for llvm::sys::Execute(And|No)Wait
ClosedPublic

Authored by alexfh on Sep 7 2017, 5:03 AM.

Details

Summary

Change the type of the Redirects parameter of llvm::sys::ExecuteAndWait,
ExecuteNoWait and other APIs that wrap them from const StringRef ** to
ArrayRef<Optional<StringRef>>, which is safer and simplifies the use of these
APIs (no more local StringRef variables just to get a pointer to).

Corresponding clang changes will be posted as a separate patch.

Event Timeline

alexfh created this revision.Sep 7 2017, 5:03 AM
alexfh updated this revision to Diff 114159.Sep 7 2017, 5:06 AM

Make default arguments consistent (None -> {}).

vsk added a subscriber: vsk.Sep 8 2017, 4:46 PM

It'd be nice to add in "expected 3 redirects" to the assert bodies.

tools/llvm-cov/CodeCoverage.cpp
460

You've used llvm::None for the stderr redirect, and I think it would be clearer / more consistent to use llvm::None here too.

vsk added inline comments.Sep 8 2017, 4:54 PM
tools/llvm-cov/CodeCoverage.cpp
460

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.

alexfh added inline comments.Sep 13 2017, 7:53 AM
tools/llvm-cov/CodeCoverage.cpp
460

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.

alexfh marked 3 inline comments as done.Sep 13 2017, 7:53 AM
bkramer accepted this revision.Sep 13 2017, 8:29 AM

lgtm

This revision is now accepted and ready to land.Sep 13 2017, 8:29 AM
This revision was automatically updated to reflect the committed changes.
In D37563#865541, @vsk wrote:

It'd be nice to add in "expected 3 redirects" to the assert bodies.

Sorry, missed this comment somehow. If we add a message, it should also cover the case of zero redirects. Something along the lines of "expected 0 or 3 redirects". I have a slight concern that the message would basically repeat the condition resulting in a sort of tautology (assert((Redirects.empty() || Redirects.size() == 3) && "expected 0 or 3 redirects");). But if you still find this useful, I can add a message.

vsk added a comment.Sep 14 2017, 9:28 AM
In D37563#865541, @vsk wrote:

It'd be nice to add in "expected 3 redirects" to the assert bodies.

Sorry, missed this comment somehow. If we add a message, it should also cover the case of zero redirects. Something along the lines of "expected 0 or 3 redirects". I have a slight concern that the message would basically repeat the condition resulting in a sort of tautology (assert((Redirects.empty() || Redirects.size() == 3) && "expected 0 or 3 redirects");). But if you still find this useful, I can add a message.

I see, it looks fine as-is.

vsk added a comment.Sep 14 2017, 1:38 PM

Actually could you add a blurb about this to docs/ReleaseNotes? I had to fix some old code in lldb and stuff we have internally. It'd be useful to add a sentence or two about how to upgrade old callsites.

In D37563#871083, @vsk wrote:

Actually could you add a blurb about this to docs/ReleaseNotes? I had to fix some old code in lldb and stuff we have internally. It'd be useful to add a sentence or two about how to upgrade old callsites.

Done in r313356.