Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

evanlucas
Copy link
Contributor

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

  • Migrate debug option parsing
  • Make sure the --icu-data-dir flag works as intended

Questions

  1. Is this a direction that we are interested in going?
  2. This library throws exceptions in the event a flag that requires an argument fails to validate. So, it requires removing the -fno-exceptions flag. Is that acceptable?

In general, I would appreciate any/all feedback to see if this is something that should be pursued.

@silverwind
Copy link
Contributor

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.

@evanlucas
Copy link
Contributor Author

The only change made was to allow getting the unmatched flags. Otherwise, we would have to define all of the v8 flags individually :/

@bnoordhuis
Copy link
Member

This library throws exceptions in the event a flag that requires an argument fails to validate. So, it requires removing the -fno-exceptions flag. Is that acceptable?

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?

@brendanashworth brendanashworth added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 18, 2015
@evanlucas
Copy link
Contributor Author

Maybe something more getopt-like?

@Fishrock123
Copy link
Contributor

@bnoordhuis mentioned two options here: #1691 (comment)

@Fishrock123
Copy link
Contributor

See also #1197

@evanlucas
Copy link
Contributor Author

I guess the biggest difficulty I'm seeing is distinguishing between v8 arguments and arguments that should be in argv vs exec_argv. Not sure how trivial of a task this is going to be. I also see moving this from the current implementation to have breaking changes.

@evanlucas
Copy link
Contributor Author

closing in favor of #1804

@evanlucas evanlucas closed this May 26, 2015
brendanashworth referenced this pull request in brendanashworth/io.js May 28, 2015
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.
brendanashworth referenced this pull request May 30, 2015
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.

PR-URL: #1819
Reviewed-By: Ben Noordhuis <[email protected]>
ryzokuken added a commit to ryzokuken/node that referenced this pull request Mar 6, 2018
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
devsnek pushed a commit that referenced this pull request Mar 8, 2018
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
targos pushed a commit that referenced this pull request Mar 17, 2018
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@targos targos mentioned this pull request Mar 18, 2018
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
trivikr pushed a commit to trivikr/node that referenced this pull request Sep 15, 2018
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants