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

deps: move more deprecations to V8_DEPRECATED #23414

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

These APIs have been deprecated upstream in V8.

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

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Oct 10, 2018
@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 10, 2018
@refack
Copy link
Contributor

refack commented Oct 10, 2018

Is it really more efficient to keep floating patches when we can turn on V8_IMMINENT_DEPRECATION_WARNINGS?

 common.gypi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/common.gypi b/common.gypi
index 4c43db48f3..564ec316a8 100644
--- a/common.gypi
+++ b/common.gypi
@@ -264,6 +264,10 @@
         }
       }
     },
+    'defines': [
+      'V8_DEPRECATION_WARNINGS',
+      'V8_IMMINENT_DEPRECATION_WARNINGS',
+    ],
     # Forcibly disable -Werror.  We support a wide range of compilers, it's
     # simply not feasible to squelch all warnings, never mind that the
     # libraries in deps/ are not under our control.

@addaleax
Copy link
Member Author

@refack How much compiler warning output would we currently get with those settings?

@refack
Copy link
Contributor

refack commented Oct 11, 2018

Just from /src/ (trimming duplicates)

Number Method
3 v8::Date::New #23288
2 v8::Function::Call
29 v8::Object::Get
1 v8::Object::GetPropertyNames
147 v8::Object::Set
2 v8::String::NewFromTwoByte
2 v8::Value::ToObject
(2) v8::WasmCompiledModule::CallerOwnedBuffer

We can also consider setting it only for the "SDK" (i.e. common.gypi to be used by native addons).

refack pushed a commit to RomainForks/node that referenced this pull request Oct 23, 2018
Refs: nodejs#23414 (comment)

PR-URL: nodejs#23804
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
These APIs have been deprecated upstream in V8.
@addaleax
Copy link
Member Author

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 24, 2018
targos pushed a commit that referenced this pull request Oct 24, 2018
Refs: #23414 (comment)

PR-URL: #23804
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax
Copy link
Member Author

Landed in 4fe0bb3

@addaleax addaleax closed this Oct 25, 2018
@addaleax addaleax deleted the v8-dep-again branch October 25, 2018 18:22
addaleax added a commit that referenced this pull request Oct 25, 2018
These APIs have been deprecated upstream in V8.

PR-URL: #23414
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Oct 26, 2018
These APIs have been deprecated upstream in V8.

PR-URL: #23414
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Nov 1, 2018
These APIs have been deprecated upstream in V8.

PR-URL: #23414
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
Refs: #23414 (comment)

PR-URL: #23804
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Refs: #23414 (comment)

PR-URL: #23804
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Refs: #23414 (comment)

PR-URL: #23804
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. semver-minor PRs that contain new features and should be released in the next minor version. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants