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.

Sign up for GitHub

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

cli: normalize _ → - when parsing options #23020

Closed
wants to merge 4 commits into from
+45 −58
Closed

cli: normalize _ → - when parsing options #23020

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
671173e
cli: normalize `_` → `-` when parsing options
addaleax Sep 22, 2018
841cf93
fixup! cli: normalize `_` → `-` when parsing options
addaleax Sep 22, 2018
a389c08
fixup! cli: normalize `_` → `-` when parsing options
addaleax Sep 22, 2018
7ca87b0
fixup! cli: normalize `_` → `-` when parsing options
addaleax Sep 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions doc/api/cli.md
View file
Original file line number Diff line number Diff line change
@@ -21,6 +21,18 @@ Execute without arguments to start the [REPL][].
_For more info about `node inspect`, please see the [debugger][] documentation._

## Options
<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/23020
description: Underscores instead of dashes are now allowed for
Node.js options as well, in addition to V8 options.
-->

All options, including V8 options, allow words to be separated by both
dashes (`-`) or underscores (`_`).

For example, `--pending-deprecation` is equivalent to `--pending_deprecation`.

### `-`
<!-- YAML
@@ -390,11 +402,6 @@ added: v0.1.3

Print V8 command line options.

V8 options allow words to be separated by both dashes (`-`) or
underscores (`_`).

For example, `--stack-trace-limit` is equivalent to `--stack_trace_limit`.

### `--v8-pool-size=num`
<!-- YAML
added: v5.10.0
43 changes: 12 additions & 31 deletions lib/internal/bootstrap/node.js
View file
Original file line number Diff line number Diff line change
@@ -713,7 +713,7 @@
// This builds process.allowedNodeEnvironmentFlags
// from data in the config binding

const replaceDashesRegex = /-/g;
const replaceUnderscoresRegex = /_/g;
const leadingDashesRegex = /^--?/;
const trailingValuesRegex = /=.*$/;

@@ -725,20 +725,14 @@
const get = () => {
const {
getOptions,
types: { kV8Option },
envSettings: { kAllowedInEnvironment }
} = internalBinding('options');
const { options, aliases } = getOptions();

const allowedV8EnvironmentFlags = [];
const allowedNodeEnvironmentFlags = [];
for (const [name, info] of options) {
if (info.envVarSettings === kAllowedInEnvironment) {
if (info.type === kV8Option) {
allowedV8EnvironmentFlags.push(name);
} else {
allowedNodeEnvironmentFlags.push(name);
}
allowedNodeEnvironmentFlags.push(name);
}
}

@@ -772,11 +766,9 @@
// process.allowedNodeEnvironmentFlags.has() which lack leading dashes.
// Avoid interference w/ user code by flattening `Set.prototype` into
// each object.
const [nodeFlags, v8Flags] = [
allowedNodeEnvironmentFlags, allowedV8EnvironmentFlags
].map((flags) => Object.defineProperties(
new Set(flags.map(trimLeadingDashes)),
Object.getOwnPropertyDescriptors(Set.prototype))
const nodeFlags = Object.defineProperties(
new Set(allowedNodeEnvironmentFlags.map(trimLeadingDashes)),
Object.getOwnPropertyDescriptors(Set.prototype)
);

class NodeEnvironmentFlagsSet extends Set {
@@ -800,29 +792,18 @@
has(key) {
// This will return `true` based on various possible
// permutations of a flag, including present/missing leading
// dash(es) and/or underscores-for-dashes in the case of V8-specific
// flags. Strips any values after `=`, inclusive.
// dash(es) and/or underscores-for-dashes.
// Strips any values after `=`, inclusive.
// TODO(addaleax): It might be more flexible to run the option parser
// on a dummy option set and see whether it rejects the argument or
// not.
if (typeof key === 'string') {
key = replace(key, trailingValuesRegex, '');
key = replace(key, replaceUnderscoresRegex, '-');
if (test(leadingDashesRegex, key)) {
return has(this, key) ||
has(v8Flags,
replace(
replace(
key,
leadingDashesRegex,
''
),
replaceDashesRegex,
'_'
)
);
key = replace(key, trailingValuesRegex, '');
return has(this, key);
}
return has(nodeFlags, key) ||
has(v8Flags, replace(key, replaceDashesRegex, '_'));
return has(nodeFlags, key);
}
return false;
}
@@ -833,7 +814,7 @@

return process.allowedNodeEnvironmentFlags = Object.freeze(
new NodeEnvironmentFlagsSet(
allowedNodeEnvironmentFlags.concat(allowedV8EnvironmentFlags)
allowedNodeEnvironmentFlags
));
};

20 changes: 7 additions & 13 deletions src/node_options-inl.h
View file
Original file line number Diff line number Diff line change
@@ -307,6 +307,13 @@ void OptionsParser<Options>::Parse(
if (equals_index != std::string::npos)
original_name += '=';

{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO should be:

for(auto& i: name) {
  if (i == '_') i = '-';
}

So there's no need for the scope, and no use of while when there's an iterator.
ES.71

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still have as a rule here to avoid using non-const references., and the rule you linked doesn’t seem to apply here (this is not a for loop to begin with). If you want, this can be

for (std::string::size_type i = 0; i < name.size(); ++i) {
  if (name[i] == '_') 
    name[i] = '-';
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like the for variant better. It's simple, and probably has comparable performance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P.S. There ES.72 - Prefer a for-statement to a while-statement when there is an obvious loop variable

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which for variant? The no-non-const-references rule has a good reason, which is that it should be obvious what is being modified (and in your code that is name and not just i itself).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The one you suggested.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The no-non-const-references rule has a good reason

I have no doubt rules have good reasons. Personally I'm assuming anything that is not const is going to be changed, but I'd be happy to discuss this in the context of updating the c++ style guide.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The no-non-const-references rule

Where can I find a reference to that?

// Normalize by replacing `_` with `-` in options.
std::string::size_type index = 2; // Start after initial '--'.
while ((index = name.find('_', index + 1)) != std::string::npos)
name[index] = '-';
}

{
auto it = aliases_.end();
// Expand aliases:
@@ -341,19 +348,6 @@ void OptionsParser<Options>::Parse(

auto it = options_.find(name);

if (it == options_.end()) {
// We would assume that this is a V8 option if neither we nor any child
// parser knows about it, so we convert - to _ for
// canonicalization (since V8 accepts both) and look up again in order
// to find a match.
// TODO(addaleax): Make the canonicalization unconditional, i.e. allow
// both - and _ in Node's own options as well.
std::string::size_type index = 2; // Start after initial '--'.
while ((index = name.find('-', index + 1)) != std::string::npos)
name[index] = '_';
it = options_.find(name);
}

if ((it == options_.end() ||
it->second.env_setting == kDisallowedInEnvironment) &&
required_env_settings == kAllowedInEnvironment) {
12 changes: 5 additions & 7 deletions src/node_options.cc
View file
Original file line number Diff line number Diff line change
@@ -102,8 +102,6 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
&EnvironmentOptions::experimental_worker,
kAllowedInEnvironment);
AddOption("--expose-internals", "", &EnvironmentOptions::expose_internals);
// TODO(addaleax): Remove this when adding -/_ canonicalization to the parser.
AddAlias("--expose_internals", "--expose-internals");
AddOption("--loader",
"(with --experimental-modules) use the specified file as a "
"custom loader",
@@ -204,15 +202,15 @@ PerIsolateOptionsParser::PerIsolateOptionsParser() {
kAllowedInEnvironment);

// Explicitly add some V8 flags to mark them as allowed in NODE_OPTIONS.
AddOption("--abort_on_uncaught_exception",
AddOption("--abort-on-uncaught-exception",
"aborting instead of exiting causes a core file to be generated "
"for analysis",
V8Option{},
kAllowedInEnvironment);
AddOption("--max_old_space_size", "", V8Option{}, kAllowedInEnvironment);
AddOption("--perf_basic_prof", "", V8Option{}, kAllowedInEnvironment);
AddOption("--perf_prof", "", V8Option{}, kAllowedInEnvironment);
AddOption("--stack_trace_limit", "", V8Option{}, kAllowedInEnvironment);
AddOption("--max-old-space-size", "", V8Option{}, kAllowedInEnvironment);
AddOption("--perf-basic-prof", "", V8Option{}, kAllowedInEnvironment);
AddOption("--perf-prof", "", V8Option{}, kAllowedInEnvironment);
AddOption("--stack-trace-limit", "", V8Option{}, kAllowedInEnvironment);

Insert(&EnvironmentOptionsParser::instance,
&PerIsolateOptions::get_per_env_options);
1 change: 0 additions & 1 deletion test/parallel/test-cli-node-options-disallowed.js
View file
Original file line number Diff line number Diff line change
@@ -29,7 +29,6 @@ disallow('--interactive');
disallow('-i');
disallow('--v8-options');
disallow('--');
disallow('--no_warnings'); // Node options don't allow '_' instead of '-'.

function disallow(opt) {
const env = Object.assign({}, process.env, { NODE_OPTIONS: opt });
1 change: 1 addition & 0 deletions test/parallel/test-cli-node-options.js
View file
Original file line number Diff line number Diff line change
@@ -19,6 +19,7 @@ expect(`-r ${printA}`, 'A\nB\n');
expect(`-r ${printA} -r ${printA}`, 'A\nB\n');
expect('--no-deprecation', 'B\n');
expect('--no-warnings', 'B\n');
expect('--no_warnings', 'B\n');
expect('--trace-warnings', 'B\n');
expect('--redirect-warnings=_', 'B\n');
expect('--trace-deprecation', 'B\n');
8 changes: 8 additions & 0 deletions test/parallel/test-pending-deprecation.js
View file
Original file line number Diff line number Diff line change
@@ -36,6 +36,14 @@ switch (process.argv[2]) {
assert.strictEqual(code, 0, message('--pending-deprecation'));
}));

// Test the --pending_deprecation command line switch.
fork(__filename, ['switch'], {
execArgv: ['--pending_deprecation'],
silent: true
}).on('exit', common.mustCall((code) => {
assert.strictEqual(code, 0, message('--pending_deprecation'));
}));

// Test the NODE_PENDING_DEPRECATION environment var.
fork(__filename, ['env'], {
env: Object.assign({}, process.env, { NODE_PENDING_DEPRECATION: 1 }),
2 changes: 1 addition & 1 deletion test/parallel/test-process-env-allowed-flags.js
View file
Original file line number Diff line number Diff line change
@@ -19,10 +19,10 @@ require('../common');
].concat(process.config.variables.v8_enable_inspector ? [
'--inspect-brk',
'inspect-brk',
'--inspect_brk',
] : []);

const badFlags = [
'--inspect_brk',
'INSPECT-BRK',
'--INSPECT-BRK',
'--r',