Last Comment Bug 668095 - Write a proper option parser for SpiderMonkey CLI
: Write a proper option parser for SpiderMonkey CLI
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Chris Leary [:cdleary] (not checking bugmail)
:
Mentors:
: 622178 668350 (view as bug list)
Depends on: 673052 894240
Blocks: 671442
  Show dependency treegraph
 
Reported: 2011-06-28 16:31 PDT by Chris Leary [:cdleary] (not checking bugmail)
Modified: 2013-07-16 00:04 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP: passes JS tests and trace tests. (26.79 KB, patch)
2011-07-05 14:41 PDT, Chris Leary [:cdleary] (not checking bugmail)
no flags Details | Diff | Review
WIP: prints a little auto-formatted help dialogue. (32.25 KB, patch)
2011-07-05 20:07 PDT, Chris Leary [:cdleary] (not checking bugmail)
no flags Details | Diff | Review
WIP (33.24 KB, patch)
2011-07-05 20:29 PDT, Chris Leary [:cdleary] (not checking bugmail)
dvander: feedback+
Details | Diff | Review
WIP (49.51 KB, patch)
2011-07-08 16:21 PDT, Chris Leary [:cdleary] (not checking bugmail)
no flags Details | Diff | Review
OptionParser (48.74 KB, patch)
2011-07-12 15:12 PDT, Chris Leary [:cdleary] (not checking bugmail)
dvander: review-
Details | Diff | Review
OptionParser, take 2 (53.59 KB, patch)
2011-07-14 15:02 PDT, Chris Leary [:cdleary] (not checking bugmail)
dvander: review+
Details | Diff | Review
follow-up (7.43 KB, patch)
2011-07-14 20:03 PDT, David Anderson [:dvander]
cdleary: review+
Details | Diff | Review

Description Chris Leary [:cdleary] (not checking bugmail) 2011-06-28 16:31:44 PDT
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) 2011-07-05 14:41:41 PDT
Created attachment 544067 [details] [diff] [review]
WIP: passes JS tests and trace tests.
Comment 2 Chris Leary [:cdleary] (not checking bugmail) 2011-07-05 20:07:15 PDT
Created attachment 544126 [details] [diff] [review]
WIP: prints a little auto-formatted help dialogue.
Comment 3 Chris Leary [:cdleary] (not checking bugmail) 2011-07-05 20:29:18 PDT
Created attachment 544132 [details] [diff] [review]
WIP

f?=dmandelin for feedback on which options to keep.
Comment 4 Chris Leary [:cdleary] (not checking bugmail) 2011-07-06 15:55:15 PDT
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] 2011-07-06 15:58:57 PDT
Comment on attachment 544132 [details] [diff] [review]
WIP

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

Yeah, this looks perfect.
Comment 6 Ryan Pearl [:rpearl] 2011-07-06 16:07:36 PDT
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) 2011-07-06 16:11:07 PDT
(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] 2011-07-07 13:56:48 PDT
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 (:billm) 2011-07-07 14:01:06 PDT
I use -b and -Z. Please don't remove them.
Comment 10 Chris Leary [:cdleary] (not checking bugmail) 2011-07-08 16:21:14 PDT
Created attachment 544928 [details] [diff] [review]
WIP
Comment 11 Chris Leary [:cdleary] (not checking bugmail) 2011-07-08 16:56:24 PDT
sfink says he wants to keep the recently-added -D --disassemble as well.
Comment 12 Chris Leary [:cdleary] (not checking bugmail) 2011-07-12 15:12:32 PDT
Created attachment 545502 [details] [diff] [review]
OptionParser

Note that we no longer bind an arguments array on the global object.
Comment 13 David Anderson [:dvander] 2011-07-13 16:13:44 PDT
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) 2011-07-14 15:02:58 PDT
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.
Comment 15 David Anderson [:dvander] 2011-07-14 17:44:16 PDT
Comment on attachment 546009 [details] [diff] [review]
OptionParser, take 2

Awesome!
Comment 16 David Anderson [:dvander] 2011-07-14 18:46:50 PDT
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] 2011-07-14 20:03:36 PDT
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?
Comment 18 Marco Bonardo [::mak] 2011-07-20 06:58:17 PDT
http://hg.mozilla.org/mozilla-central/rev/b1923b866d6a
Comment 19 Andrew Drake [:adrake] 2011-07-28 22:46:32 PDT
*** Bug 668350 has been marked as a duplicate of this bug. ***
Comment 20 Chris Leary [:cdleary] (not checking bugmail) 2011-12-20 16:27:24 PST
*** Bug 622178 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.