Closed
Bug 668095
Opened 13 years ago
Closed 13 years ago
Write a proper option parser for SpiderMonkey CLI
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: cdleary, Assigned: cdleary)
References
Details
Attachments
(2 files, 5 obsolete files)
53.59 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
7.43 KB,
patch
|
cdleary
:
review+
|
Details | Diff | Splinter Review |
I've got a patch in progress that has js.cpp using an option parser in the style of Python's optparse (mostly because that's the only option parser I've ever used that didn't totally suck).
Assignee | ||
Comment 1•13 years ago
|
||
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #544067 -
Attachment is obsolete: true
Assignee | ||
Comment 3•13 years ago
|
||
f?=dmandelin for feedback on which options to keep.
Attachment #544126 -
Attachment is obsolete: true
Attachment #544132 -
Flags: feedback?(dmandelin)
Assignee | ||
Comment 4•13 years ago
|
||
Comment on attachment 544132 [details] [diff] [review] WIP dvander, could you add some examples of flags that you'd like to add so I know it supports the use cases you want?
Attachment #544132 -
Flags: feedback?(dvander)
Comment on attachment 544132 [details] [diff] [review] WIP Review of attachment 544132 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, this looks perfect.
Attachment #544132 -
Flags: feedback?(dvander) → feedback+
Comment 6•13 years ago
|
||
One use-case which isn't covered yet is that there are three levels for GVN: off, one-pass, and fixed-point; only one of these can be set at a time. I guess either specifying some option from a set, or just an integer (but I'd need to be able to specify 0 == "off", 1 == "one-pass", 2 == "fixed-point" in terms of help strings or something like that).
Assignee | ||
Comment 7•13 years ago
|
||
(In reply to comment #6) > I guess either specifying some option from a set, or just an integer (but > I'd need to be able to specify 0 == "off", 1 == "one-pass", 2 == > "fixed-point" in terms of help strings or something like that). Sure, I'll throw in an valued option to map a string to an enum (something like addChoiceOption which will validate the choice string).
Comment 8•13 years ago
|
||
Comment on attachment 544132 [details] [diff] [review] WIP Options h and i should be preserved. I wonder if A and O might be used in some of our tests. I suspect any number of swWbtoVZ might be used by someone, but we can check on that. So please post to the mailing lists and allow a reasonable amount of time before committing.
Attachment #544132 -
Flags: feedback?(dmandelin)
I use -b and -Z. Please don't remove them.
Assignee | ||
Comment 10•13 years ago
|
||
Attachment #544132 -
Attachment is obsolete: true
Assignee | ||
Comment 11•13 years ago
|
||
sfink says he wants to keep the recently-added -D --disassemble as well.
Assignee | ||
Comment 12•13 years ago
|
||
Note that we no longer bind an arguments array on the global object.
Attachment #544928 -
Attachment is obsolete: true
Attachment #545502 -
Flags: review?(dvander)
Comment on attachment 545502 [details] [diff] [review] OptionParser Review of attachment 545502 [details] [diff] [review]: ----------------------------------------------------------------- I really like this design, it looks clean and super easy to use. The r- is just for a lost feature, but I've got some other random comments too. ::: js/src/shell/jsoptparse.cpp @@ +257,5 @@ > +OptionParser::Result > +OptionParser::extractValue(size_t argc, char **argv, size_t *i, char **value) > +{ > + JS_ASSERT(*i < argc); > + char *eq = strrchr(argv[*i], '='); Should this be strchr, in case --blah="thing=stuff" ? @@ +319,5 @@ > + } > +} > + > +OptionParser::Result > +OptionParser::parseArgs(int argc_, char **argv) blah_ instead of blah seems to be becoming popular for member variables. Could this be inputArgc or something? @@ +342,5 @@ > + } else { > + /* Short option */ > + opt = findOption(arg[1]); > + if (!opt) > + return error("Invalid short option: %s", arg); What happens if you do something like: -clam @@ +354,5 @@ > + return r; > + } > + } else { > + /* Argument. */ > + JS_ASSERT(false); We talked IRL but this is the reason for the r-, if I'm reading the code correctly. Everyone is going to want to do path/to/js file.js And I think this is really useful to preserve. @@ +395,5 @@ > +OptionParser::~OptionParser() > +{ > + for (Option **it = options.begin(), **end = options.end(); it != end; ++it) { > + (*it)->~Option(); > + js_free(*it); We don't have anything like js_destroy<T> to do this? @@ +451,5 @@ > + defaultValue); > + if (!io) > + return false; > + options.infallibleAppend(io); > + return true; Is there a reason for this pattern? return options.append(io); seems simpler ::: js/src/shell/jsoptparse.h @@ +294,5 @@ > + MultiStringOptionRange getMultiStringOption(char shortflag) const; > + > +#ifdef DEBUG > + void dump(); > +#endif Can we get rid of all these #ifdef DEBUG? And just move it to the implementation.
Attachment #545502 -
Flags: review?(dvander) → review-
Assignee | ||
Comment 14•13 years ago
|
||
Takes an optional script argument followed by arguments that get bound to global |scriptArgs|, so it can be used via a shebang line. See http://groups.google.com/group/mozilla.dev.tech.js-engine/browse_thread/thread/641db45f903dd221 for background info on that use case.
Attachment #545502 -
Attachment is obsolete: true
Attachment #546009 -
Flags: review?(dvander)
Comment on attachment 546009 [details] [diff] [review] OptionParser, take 2 Awesome!
Attachment #546009 -
Flags: review?(dvander) → review+
We've been drooling for this patch so I've landed it on ionmonkey early http://hg.mozilla.org/users/danderson_mozilla.com/ionmonkey/rev/82756b315e5a
Follow-up patch to make shortflags optional, since Ion flags are kind of long and don't have meaningful single letters. If this doesn't gross things up too much, could you fold it into your queue?
Attachment #546080 -
Flags: review?(cdleary)
Assignee | ||
Updated•13 years ago
|
Attachment #546080 -
Flags: review?(cdleary) → review+
Assignee | ||
Updated•13 years ago
|
Whiteboard: [inbound]
Comment 18•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/b1923b866d6a
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Updated•13 years ago
|
Whiteboard: [inbound]
You need to log in
before you can comment on or make changes to this bug.
Description
•