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

build: remove (almost) unused macros/constants #30755

Closed
wants to merge 3 commits into from

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Dec 1, 2019

Macros, like CHECK, cause issues for tracking coverage because
they modify the source before it's placed in V8. Upon investigation
it seemed that we only used this functionality in two places:
internal/vm/module.js, and internal/async_hooks.js (in comments).

Given this, it seemed to make more sense to move CHECK to
JavaScript, and retire a mostly unused build step.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 1, 2019
@bcoe bcoe added the build Issues and PRs related to build files or the CI. label Dec 1, 2019
@nodejs-github-bot

This comment has been minimized.

@bcoe
Copy link
Contributor Author

bcoe commented Dec 1, 2019

@nodejs/build

@bcoe
Copy link
Contributor Author

bcoe commented Dec 2, 2019

@devsnek you touched the macro logic recently, so would especially love your check to make sure this won't break anything.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

I think replacing this entirely by internal/assert is also a good idea 👍

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM with a nit. Also in case this is used in other places in the future I think it should probably be better moved to internal/assert.js instead.

On a side note the semantics here is probably more like DCHECK instead of CHECK.

lib/internal/vm/module.js Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

Macros, like CHECK, cause issues for tracking coverage because
they modify the source before it's placed in V8. Upon investigation
it seemed that we only used this functionality in two places:
internal/vm/module.js, and internal/async_hooks.js (in comments).

Given this, it seemed to make more sense to move CHECK to
JavaScript, and retire a mostly unused build step.
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

bcoe added a commit that referenced this pull request Dec 5, 2019
Macros, like CHECK, cause issues for tracking coverage because
they modify the source before it's placed in V8. Upon investigation
it seemed that we only used this functionality in two places:
internal/vm/module.js, and internal/async_hooks.js (in comments).

Given this, it seemed to make more sense to move CHECK to
JavaScript, and retire a mostly unused build step.

PR-URL: #30755
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@bcoe
Copy link
Contributor Author

bcoe commented Dec 5, 2019

Landed in bfd9de6

@bcoe bcoe closed this Dec 5, 2019
@bcoe bcoe deleted the retire-macros branch December 5, 2019 23:48
addaleax added a commit to addaleax/node that referenced this pull request Dec 6, 2019
targos pushed a commit that referenced this pull request Dec 9, 2019
Macros, like CHECK, cause issues for tracking coverage because
they modify the source before it's placed in V8. Upon investigation
it seemed that we only used this functionality in two places:
internal/vm/module.js, and internal/async_hooks.js (in comments).

Given this, it seemed to make more sense to move CHECK to
JavaScript, and retire a mostly unused build step.

PR-URL: #30755
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jan 14, 2020
Macros, like CHECK, cause issues for tracking coverage because
they modify the source before it's placed in V8. Upon investigation
it seemed that we only used this functionality in two places:
internal/vm/module.js, and internal/async_hooks.js (in comments).

Given this, it seemed to make more sense to move CHECK to
JavaScript, and retire a mostly unused build step.

PR-URL: #30755
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jan 14, 2020
Refs: #30755

PR-URL: #30815
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Macros, like CHECK, cause issues for tracking coverage because
they modify the source before it's placed in V8. Upon investigation
it seemed that we only used this functionality in two places:
internal/vm/module.js, and internal/async_hooks.js (in comments).

Given this, it seemed to make more sense to move CHECK to
JavaScript, and retire a mostly unused build step.

PR-URL: #30755
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Refs: #30755

PR-URL: #30815
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 12, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 12, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 12, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 12, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 12, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 12, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 15, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 15, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 15, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 18, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 18, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 18, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 21, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 21, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 21, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 21, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 21, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 21, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 24, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 24, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 24, 2020
codebytere added a commit to electron/electron that referenced this pull request Feb 24, 2020
* chore: bump node in DEPS to v12.16.0

* Fixup asar support setup patch

nodejs/node#30862

* Fixup InternalCallbackScope patch

nodejs/node#30236

* Fixup GN buildfiles patch

nodejs/node#30755

* Fixup low-level hooks patch

nodejs/node#30466

* Fixup globals require patch

nodejs/node#31643

* Fixup process stream patch

nodejs/node#30862

* Fixup js2c modification patch

nodejs/node#30755

* Fixup internal fs override patch

nodejs/node#30610

* Fixup context-aware warn patch

nodejs/node#30336

* Fixup Node.js with ltcg config

nodejs/node#29388

* Fixup oaepLabel patch

nodejs/node#30917

* Remove redundant ESM test patch

nodejs/node#30997

* Remove redundant cli flag patch

nodejs/node#30466

* Update filenames.json

* Remove macro generation in GN build files

nodejs/node#30755

* Fix some compilation errors upstream

* Add uvwasi to deps

nodejs/node#30258

* Fix BoringSSL incompatibilities

* Fixup linked module patch

nodejs/node#30274

* Add missing sources to GN uv build

libuv/libuv#2347

* Patch some uvwasi incompatibilities

* chore: bump Node.js to v12.6.1

* Remove mark_arraybuffer_as_untransferable.patch

nodejs/node#30549

* Fix uvwasi build failure on win

* Fixup --perf-prof cli option error

* Fixup early cjs module loading

* fix: initialize diagnostics properly

nodejs/node#30025

* Disable new esm syntax specs

nodejs/node#30219

* Fixup v8 weakref hook spec

nodejs/node#29874

* Fix async context timer issue

* Disable monkey-patch-main spec

It relies on nodejs/node#29777, and we don't
override prepareStackTrace.

* Disable new tls specs

nodejs/node#23188

We don't support much of TLS owing to schisms between BoringSSL and
OpenSSL.

Co-authored-by: Shelley Vohr <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants