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
Add -help/--help option to ocamlrun #10101
Conversation
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.
You're slicing it thin :-) Yes, it's good to have a help option for ocamlrun. Nitpicks below.
runtime/startup_byt.c
Outdated
" -m Print the magic number of <executable> and exit\n" | ||
" -M Print the magic number expected by this runtime and exit\n" | ||
" -p Print the names of the primitives known to this runtime\n" | ||
" -t Increments the trace level in debug mode by 1\n" |
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 about tracing the bytecode interpreter. Plus, "by 1" is slightly pedantic :-)
" -t Increments the trace level in debug mode by 1\n" | |
" -t Trace the execution of the bytecode interpreter. Multiple -t increase verbosity\n" |
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.
It's probablycertainly just me, but the lack of the plural on "-t" bothers me... does "Trace the execution of the bytecode interpreter (specify multiple times to increase verbosity)" work?
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.
Yes, it works fine.
runtime/startup_byt.c
Outdated
" -version Print version string and exit\n" | ||
" -vnum Print short version number and exit\n" | ||
" -help Display this list of options\n" | ||
" --help Display this list of options"); |
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.
Why no \n
at the end of this line? Oh, yes, because you're using this awful puts
function :-)
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'm sure I've submitted a patch in the past where the \n
was forgotten, but I can't find it!
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.
There was an extraneous entry in the Changes file, which I removed by a direct push.
Otherwise this is good to go! Merging right now...
No, not merging right now, as it contains other bits from other pull requests. This is not friendly to reviewers and mergers. |
Rebased without the #10098 commits with changes - good to go with CI |
Split off from #9284; this requires #10098, so only d2c2f53 needs reviewing