-
-
Notifications
You must be signed in to change notification settings - Fork 129
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
Remove the --run
flag
#2931
Remove the --run
flag
#2931
Conversation
I think it is fine, but let me try to test it by hand first, since the modifications are quite extensive in the driver. |
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 have left two comments; otherwise, this looks good. Thank you!
src/bin/lfortran.cpp
Outdated
if (0 < err && err < 256) { | ||
return err; | ||
} else { | ||
return 1; |
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.
See: lcompilers/lpython#1352 (comment)
return 1; | |
return LCompilers::get_exit_status(err); |
src/bin/lfortran.cpp
Outdated
} else { | ||
LCOMPILERS_ASSERT(false); | ||
return 1; | ||
} | ||
|
||
if (outfile != file_name + ".out" || compiler_options.arg_o != "") { |
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.
This is very rare case, but I think this might fail for the case lfortran x.f90 -o x.out
, so let's only do?
if (outfile != file_name + ".out" || compiler_options.arg_o != "") { | |
if ( compiler_options.arg_o != "" ) { |
2dccf6f
to
64a8147
Compare
Ready. |
std::string run_cmd = ""; | ||
if (backend == Backend::wasm) { | ||
// for node version less than 16, we need to also provide flag --experimental-wasm-bigint | ||
run_cmd = "node --experimental-wasi-unstable-preview1 " + outfile + ".js"; |
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 know if we want to support all the wasm runtimes. I usually use wasmtime for example.
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 think that this is fine enough to merge. If we discover any bugs, we'll fix them.
fixes #2833 (comment)