Write a proper option parser for SpiderMonkey CLI

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: cdleary, Assigned: cdleary)

Tracking

unspecified
mozilla8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

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).
Created attachment 544067 [details] [diff] [review]
WIP: passes JS tests and trace tests.
Created attachment 544126 [details] [diff] [review]
WIP: prints a little auto-formatted help dialogue.
Attachment #544067 - Attachment is obsolete: true
Created attachment 544132 [details] [diff] [review]
WIP

f?=dmandelin for feedback on which options to keep.
Attachment #544126 - Attachment is obsolete: true
Attachment #544132 - Flags: feedback?(dmandelin)
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

6 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).
(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 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.
Created attachment 544928 [details] [diff] [review]
WIP
Attachment #544132 - Attachment is obsolete: true
sfink says he wants to keep the recently-added -D --disassemble as well.
Created attachment 545502 [details] [diff] [review]
OptionParser

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-

Updated

6 years ago
Blocks: 671442
Created attachment 546009 [details] [diff] [review]
OptionParser, take 2

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
Created attachment 546080 [details] [diff] [review]
follow-up

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)
Attachment #546080 - Flags: review?(cdleary) → review+
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/b1923b866d6a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Depends on: 673052
Whiteboard: [inbound]

Updated

6 years ago
Duplicate of this bug: 668350
Duplicate of this bug: 622178
Depends on: 894240
You need to log in before you can comment on or make changes to this bug.