668095 - Write a proper option parser for SpiderMonkey CLI

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Leary [:cdleary] (not checking bugmail)]

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).

Comment 1

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: WIP: passes JS tests and trace tests.

Comment 2

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: WIP: prints a little auto-formatted help dialogue.

Comment 3

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: WIP

f?=dmandelin for feedback on which options to keep.

Comment 4

[Chris Leary [:cdleary] (not checking bugmail)]

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?

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 544132 [details] [diff] [review]
WIP

Review of attachment 544132 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, this looks perfect.

Comment 6

[Ryan Pearl [:rpearl]]

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).

Comment 7

[Chris Leary [:cdleary] (not checking bugmail)]

(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

[David Mandelin [:dmandelin]]

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.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I use -b and -Z. Please don't remove them.

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: WIP

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

sfink says he wants to keep the recently-added -D --disassemble as well.

Comment 12

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: OptionParser

Note that we no longer bind an arguments array on the global object.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 14

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 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.

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 546009 [details] [diff] [review]
OptionParser, take 2

Awesome!

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

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

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: 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?

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/b1923b866d6a