Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upsrc: refactor options parsing #22392
Conversation
addaleax
added
semver-minor
cli
labels
Aug 18, 2018
This comment has been minimized.
This comment has been minimized.
nodejs-github-bot
added
C++
lib / src
labels
Aug 18, 2018
refack
requested changes
Aug 18, 2018
| @@ -72,35 +72,38 @@ static void Initialize(Local<Object> target, | |||
| READONLY_BOOLEAN_PROPERTY("hasTracing"); | |||
| #endif | |||
|
|
|||
| READONLY_STRING_PROPERTY(target, "icuDataDir", icu_data_dir); | |||
| // TODO(addaleax): This seems to be an unused, private API. Remove it? | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
addaleax
Aug 18, 2018
Author
Member
It’s only accessible through process.binding('config'), so it’s still going to have to change somehow, assuming we push through with the process.binding() deprecations…
refack
reviewed
Aug 18, 2018
|
I love it! Way way way overdue refactoring. |
refack
dismissed
their
stale review
Aug 18, 2018
Downgrading to comment
refack
reviewed
Aug 18, 2018
| // These methods add a single option to the parser. Optionally, it can be | ||
| // specified whether the option should be allowed from environment variable | ||
| // sources (i.e. NODE_OPTIONS). | ||
| void AddOption(const std::string& name, |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
addaleax
Aug 18, 2018
Author
Member
The way this is done is because we use it to infer the info.type value. We could do something with templates but I think it would mean we’d still use a list of template specializations?
This comment has been minimized.
This comment has been minimized.
I don’t really understand this comment, can you explain or give examples? |
refack
reviewed
Aug 18, 2018
|
|
||
| namespace node { | ||
|
|
||
| DebugOptionsParser::DebugOptionsParser() { |
This comment has been minimized.
This comment has been minimized.
refack
Aug 18, 2018
Member
AFAIR this could be merged with EnvironmentOptionsParser. It was encapsulated as in preparation to something like this PR.
This comment has been minimized.
This comment has been minimized.
addaleax
Aug 18, 2018
Author
Member
Yes, this could be merged. As you are saying, it helps with encapsulation, so I don’t mind it either way.
This comment has been minimized.
This comment has been minimized.
refack
Aug 18, 2018
Member
I would add a comment stating this, just so that future generations won't assume it's something special.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Fantastic start on this. One the things I would really like to see is a quick way of determining if the options have changed from one run to another (essentially a hash value calculated from the options values) |
This comment has been minimized.
This comment has been minimized.
|
@jasnell Just so I understand it, something that one could get today |
This comment has been minimized.
This comment has been minimized.
|
Yes and no. Env vars also need to be taken into consideration and input should be normalized so that order doesn't matter (except in those handful of cases where it does, of course). It also needs to take into consideration default values that may happen to change. |
This comment has been minimized.
This comment has been minimized.
I mean implementing a generic agrv parser. I don't have enough production experience with any specific library but https://github.com/search?l=C%2B%2B&q=argument+parser&type=Repositories show afew with > 100 stars. AddOption("--inspect-port", &DebugOptions::host_port,
kAllowedInEnvironment);
AddAlias("--debug-port", "--inspect-port");
AddOption("--inspect", &DebugOptions::inspector_enabled,
kAllowedInEnvironment);
AddAlias("--inspect=", { "--inspect-port", "--inspect" });
AddOption("--debug", &DebugOptions::deprecated_debug);
AddAlias("--debug=", { "--inspect-port", "--debug" });we would have something like Option<"inspect-port", kAllowedInEnvironment> host_port;
Alias<"inspect-port"> debug-port;that might requires reflection, or c'tors with side effects, or macros... So in that sense your implementation is quite minimal, it's just that most of that information is known in compile time, so there no need to delegate it to run time. |
This comment has been minimized.
This comment has been minimized.
I think that’s not doable given how weird some of our options are (think debug flags or
I agree that there is unnecessary runtime overhead here that I’d like to get down a bit, yes. |
refack
reviewed
Aug 20, 2018
| bool Options::* field, | ||
| OptionEnvvarSettings env_setting) { | ||
| options_.emplace(name, OptionInfo { | ||
| kBoolean, |
This comment has been minimized.
This comment has been minimized.
refack
Aug 20, 2018
Member
Just thinking out loud... (It is an interesting problem after all).
How about instead on a const you put a pointer to the conversion function. That way you make these Templates specializations on the typeof(field), and eliminate the switch in L385.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
addaleax
Aug 20, 2018
Author
Member
Had a few more "mind itches". I'll try to write up a draft.
Sounds good, looking forward to it!
mcollina
approved these changes
Aug 20, 2018
|
LGTM. Can you also do a CITGM run? |
This comment has been minimized.
This comment has been minimized.
|
Yup, sounds like a good idea: CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1517/ |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@boneskull If this PR stays in its current shape, it shouldn’t be too hard – we could go through the |
This comment has been minimized.
This comment has been minimized.
|
@refack Ping … anything you’d definitely like to see changed before merging this? It would be nice to get this in and work on expanding it (programmatic accessibility/help texts). :) |
refack
approved these changes
Aug 21, 2018
This comment has been minimized.
This comment has been minimized.
No reason to wait. If I find the time we could iterate later. |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
@refack I think it might just be that New CI: https://ci.nodejs.org/job/node-test-pull-request/16655/ |
refack
reviewed
Aug 22, 2018
|
|
||
| inline int ParseAndValidatePort(const std::string& port, std::string* error) { | ||
| char* endptr; | ||
| errno = 0; |
This comment has been minimized.
This comment has been minimized.
refack
Aug 22, 2018
Member
Isn't this lint? How did this compile before? Ahh the wonder of C++ compilers...
And in the next line could we switch to strtoul?
This comment has been minimized.
This comment has been minimized.
addaleax
Aug 22, 2018
Author
Member
Lint? It’s just a missing include, I assume… and, yes, I think we could switch, but it shouldn’t make any difference?
This comment has been minimized.
This comment has been minimized.
refack
Aug 22, 2018
Member
It’s just a missing include
ahh it's defined in errno.h (that's a POSIX mechanism). Without that context it does look like an undefined var (who's value magicly changes).
This comment has been minimized.
This comment has been minimized.
|
Landed in 29a71ba |
addaleax commentedAug 18, 2018
In case anybody was wondering what I’ve been up to this week.😸
This is a major refactor of our Node’s parser. See
node_options.ccfor how it is used, and
node_options-inl.hfor the bulkof its implementation.
Unfortunately, the implementation has come to have some
complexity, in order to meet the following goals:
through both
--foo=barnotation and--foo barnotation.We were previously very inconsistent on this point.
(Labelling semver-minor because of this).
per-process (global), per-Isolate and per-Environment
(+ debug options).
--helpoutput.This commit also leaves a number of
TODOcomments, mostly forimproving consistency even more (possibly with having to modify
tests), improving embedder support, as well as removing pieces of
exposed configuration variables that should never have become
part of the public API but unfortunately are at this point.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesFyi @boneskull, you probably want to be aware of this because of #19335. :)