-
Notifications
You must be signed in to change notification settings - Fork 30k
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 more robust option parsing [WIP] #1726
Conversation
My gut feeling tells me that if you have to modify the parser, it may not be the right tool for the job. What were the issues with the v8 options? Generally, I'm +1 for a better parser that can take arguments in any order. |
The only change made was to allow getting the unmatched flags. Otherwise, we would have to define all of the v8 flags individually :/ |
I'd say no. If nothing else, it's going to add several hundreds of kilobytes of mostly unused unwind info to the binary. You could work around that by breaking out the option parsing code in node.cc into a separate file and only enabling exceptions for that particular file. You probably have to turn it into a separate static_library target for that to work. Also, 5,000 lines of mostly unvetted code... surely there must be something simpler? |
Maybe something more getopt-like? |
@bnoordhuis mentioned two options here: #1691 (comment) |
See also #1197 |
I guess the biggest difficulty I'm seeing is distinguishing between v8 arguments and arguments that should be in |
closing in favor of #1804 |
The Socket writable only change was added and implemented in the constructor around 5885f46, but this was never removed. The libev counter issue is no longer prudent; the test remains in test/sequential/test-regress-GH-1726.
Rename the test appropriately alongside mentioning the subsystem Also, make a few basic changes to make sure the test conforms to the standard test structure Refs: nodejs#19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
Rename the test appropriately alongside mentioning the subsystem Also, make a few basic changes to make sure the test conforms to the standard test structure Refs: #19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure PR-URL: #19161 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Rename the test appropriately alongside mentioning the subsystem Also, make a few basic changes to make sure the test conforms to the standard test structure Refs: #19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure PR-URL: #19161 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Rename the test appropriately alongside mentioning the subsystem Also, make a few basic changes to make sure the test conforms to the standard test structure Refs: #19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure PR-URL: #19161 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Rename the test appropriately alongside mentioning the subsystem Also, make a few basic changes to make sure the test conforms to the standard test structure Refs: nodejs#19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure PR-URL: nodejs#19161 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Rename the test appropriately alongside mentioning the subsystem Also, make a few basic changes to make sure the test conforms to the standard test structure Refs: nodejs#19105 Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure PR-URL: nodejs#19161 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
This is a start for adding more robust option parsing. I wanted to go ahead and open this to get some feedback on the direction.
Note: this is by no means complete and there are failing tests from it currently
I have included the option parser found at https://github.com/JGiard/tclap. I had to make some modifications to allow passing through v8 options.
TODO
--icu-data-dir
flag works as intendedQuestions
-fno-exceptions
flag. Is that acceptable?In general, I would appreciate any/all feedback to see if this is something that should be pursued.