-
Notifications
You must be signed in to change notification settings - Fork 273
run now does I/O redirection on Windows #2849
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
d66476c
to
8a93e41
Compare
src/util/run.cpp
Outdated
{ | ||
std::vector<std::string> new_argv = argv; | ||
new_argv.insert(new_argv.begin(), "cl.exe"); | ||
new_argv.insert(new_argv.begin() + 1, "/c"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think run
was meant to be limited to cl.exe
?! It should equally be usable, e.g., with an SMT solver?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SMT solvers are currently called with system(), but it would actually be a good idea to change this (will do separate PR).
Fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
It seems line 116 still has cl.exe
hard-coded.
8a93e41
to
b5fd869
Compare
src/util/run.cpp
Outdated
} | ||
|
||
// this is recursive | ||
return run("cl.exe", new_argv, "", "", ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there's still this invocation of cl.exe
left to be resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, now fixed.
b5fd869
to
9c22b7a
Compare
No description provided.