Skip to content

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jul 5, 2019

Refs: #28558

I did not add any documentation for this flag - the intention is to dedicate it to debugging Node.js core, somewhat similar to --expose-internals and --inspect-brk-node.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 5, 2019
Copy link
Member

Choose a reason for hiding this comment

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

Might want to mention it's an undocumented debug flag here.

Copy link
Member

Choose a reason for hiding this comment

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

I think the warnings are alphabetical so this should go above --no-warnings?

Copy link
Member

Choose a reason for hiding this comment

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

Er, well, they aren't alphabetical, but they're mostly alphabetical?

Copy link
Member

Choose a reason for hiding this comment

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

… if this is supposed to be undocumented, I’d prefer not documenting it at all (i.e. no help text) over documenting that it’s undocumented, because the latter is pretty self-contradictory.

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Jul 5, 2019

Is --no-snapshot sufficient?

@joyeecheung
Copy link
Member Author

@Trott That can be somewhat ambiguous because there is also the v8 default snapshot, we use --without-snapshot and --without-node-snapshot to distinguish between the two in the configure script.

Copy link
Member

Choose a reason for hiding this comment

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

… if this is supposed to be undocumented, I’d prefer not documenting it at all (i.e. no help text) over documenting that it’s undocumented, because the latter is pretty self-contradictory.

src/node.cc Outdated
Copy link
Member

Choose a reason for hiding this comment

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

This doesn’t really make sense as a per_env option though, right? More as a per-Isolate one?

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

Removed the help text and moved it into per-isolate options

@nodejs-github-bot
Copy link
Collaborator

joyeecheung added a commit that referenced this pull request Jul 9, 2019
PR-URL: #28567
Refs: #28558
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@joyeecheung
Copy link
Member Author

Landed in 581bddc

targos pushed a commit that referenced this pull request Jul 20, 2019
PR-URL: #28567
Refs: #28558
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
This was referenced Jul 23, 2019
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