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

Jump to bottom

Revert "v8: drop v8::FunctionCallbackInfo<T>::NewTarget()" #11652

Closed
wants to merge 144 commits into from
Closed

Revert "v8: drop v8::FunctionCallbackInfo<T>::NewTarget()" #11652

wants to merge 144 commits into from

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Mar 2, 2017 •
edited
Loading

See the commit log of the reverted commit: it's a semver-minor change
that can land in the next minor release.

This reverts commit 47cbb88.

CI: https://ci.nodejs.org/job/node-test-pull-request/6657/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/591/

@bnoordhuis bnoordhuis added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 2, 2017
@nodejs-github-bot nodejs-github-bot added v6.x v8 engine Issues and PRs related to the V8 dependency. labels Mar 2, 2017
mhdawson
mhdawson approved these changes Mar 2, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@Fishrock123
Copy link
Contributor

Is there related discussion context somewhere that is not linked to?

@addaleax
Copy link
Member

addaleax commented Mar 2, 2017

@Fishrock123 Is #9293 (comment) what you are looking for?

@bnoordhuis bnoordhuis mentioned this pull request Mar 3, 2017
Merged
@bnoordhuis
Copy link
Member Author

@ofrobots From #9293 (comment):

It might be a good idea to add the a DCHECK in IsConstructCall to make sure that the equivalence relationship between is_construct_call_ & 1 and !NewTarget->IsUndefined holds.

I don't think I can do that. IsConstructCall() lives in v8.h, where DCHECK is not defined.

@MylesBorins MylesBorins force-pushed the v6.x-staging branch 2 times, most recently from b3f32f9 to e71fc70 Compare March 8, 2017 09:27
@jasnell
Copy link
Member

jasnell commented Mar 22, 2017

Ping @bnoordhuis ... other than needing a rebase, is this ready to land?

@bnoordhuis
Copy link
Member Author

Correct. Rebased although I don't understand why GH was showing a conflict, it applies cleanly.

@jasnell
Copy link
Member

jasnell commented Mar 23, 2017

Yeah, I've also seen recently cases where github shows no conflicts for PRs that don't land cleanly. Not sure what's happening there.

New CI before landing: https://ci.nodejs.org/job/node-test-pull-request/7002/

@jasnell
Copy link
Member

jasnell commented Mar 23, 2017

@bnoordhuis ... odd build failure on one of the linux boxes: https://ci.nodejs.org/job/node-test-commit-linux/8645/nodes=ubuntu1204-clang341-64/console

@bnoordhuis
Copy link
Member Author

@nodejs/build Is that perhaps an artifact from upgrading the compiler toolchain recently? The file it's complaining about, deps/v8/src/base/functional.cc, hasn't been touched by this PR. In fact, it hasn't been touched since November 2014.

@jasnell
Copy link
Member

jasnell commented Mar 23, 2017

Yeah, that's why it's odd ;) Im not really sure why that would fail all of a sudden.

@bnoordhuis bnoordhuis mentioned this pull request Mar 23, 2017
Merged
2 tasks
@addaleax addaleax mentioned this pull request Mar 25, 2017
Closed
4 tasks
mhdawson and others added 4 commits April 4, 2017 14:20
@mhdawson @MylesBorins
doc: add supported platforms list
f05982b 
Original Commit Message:
  PR-URL: nodejs#11872
  Reviewed-By: James M Snell <jasnell@gmail.com>
  Reviewed-By: Rich Trott <rtrott@gmail.com>
  Reviewed-By: Roman Reiss <me@silverwind.io>
  Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
  Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Backport-Of: nodejs#11872
PR-URL: nodejs#11943
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mhdawson @MylesBorins
doc: add supported platforms list for v6.x
76e903e 
PR-URL: nodejs#11943
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@sushi90 @MylesBorins
doc: fix broken URL to event loop guide
6486b97 
PR-URL: nodejs#11670
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mscdex @MylesBorins
stream: avoid additional validation for Buffers
078188d 
These changes result in ~50% improvement in the included benchmark.

PR-URL: nodejs#10580
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn and others added 2 commits April 13, 2017 15:11
@gibfahn @MylesBorins
tools, test: require const/let in test
daddf5a 
Backport-PR-URL: nodejs#11775
PR-URL: nodejs#10685
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@gibfahn @MylesBorins
test: use eslint to fix var->const/let
b2fc0f0 
Manually fix issues that eslint --fix couldn't do automatically.

Backport-PR-URL: nodejs#11775
PR-URL: nodejs#10685
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
silverwind and others added 22 commits April 18, 2017 19:25
@silverwind @MylesBorins
doc: add logo to README
b5c9c12 
Adds a centered logo to the README to make it a little more festive. As
centering is not possible in pure Markdown, a bit of HTML is used.

PR-URL: nodejs#12148
Ref: nodejs#6920
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@santigimeno @MylesBorins
test: fix flaky test-child-process-exec-timeout
1f732d3 
At least starting with Darwin Kernel Version 16.4.0, sending a SIGTERM
to a process that is still starting up kills it with SIGKILL instead of
SIGTERM.

PR-URL: nodejs#12159
Refs: libuv/libuv#1226
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@DavidCai1111 @MylesBorins
test: increase querystring coverage
20c38eb 
PR-URL: nodejs#12163
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@refack @MylesBorins
test: performance, remove Popen(shell=True) on Win
9520762 
not needed according to official python docs -
https://docs.python.org/2/library/subprocess.html#index-2

PR-URL: nodejs#12138
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@refack @MylesBorins
test: more robust check for location of node.exe
eafe88b 
look for the actual produced `exe` not just the directory

PR-URL: nodejs#12120
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
@Minimalistic @MylesBorins
doc: add table of contents to README.md
90caad4 
PR-URL: nodejs#11635
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@vhmth @MylesBorins
doc: fix string interpolation in Stream 'finish'
e9a6ddd 
PR-URL: nodejs#12221
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@vsemozhetbyt @MylesBorins
doc: modernize and fix code examples in https.md
db28237 
* Replace `var` by `const`.
* Comment out ellipses.
* Update code example (provide relevant file path, add missing option).

PR-URL: nodejs#12171
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danbev @MylesBorins
build: make configure print statements consistent
fce05e3 
I noticed that few of the print statements in configure have a leading
space with is not consistent with the rest of the file. Not sure if
this intentional or not so creating this commit just to bring it up
just in case.

PR-URL: nodejs#12176

Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: mhdawson - Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@danbev @MylesBorins
build: add checks for openssl configure options
cf411d3 
Currently it is possible to configure using --without-ssl and
--shared-openssl/--openssl-no-asm/openssl-fips without an error
occuring.

The commit add check for these combinations:

$ ./configure --without-ssl --shared-openssl
Error: --without-ssl is incompatible with --shared-openssl

$ ./configure --without-ssl --openssl-no-asm
Error: --without-ssl is incompatible with --openssl-no-asm

$ ./configure --without-ssl --openssl-fips=dummy
Error: --without-ssl is incompatible with --openssl-fips

PR-URL: nodejs#12175
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@Trott @MylesBorins
test: skip irrelevant test on Windows
6c4f786 
Skip test-cluster-disconnect-handles on Windows.

PR-URL: nodejs#12261
Ref: nodejs#12197 (comment)
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@dave-k @MylesBorins
test: add a second argument to assert.throws()
81fceb3 
- a regular expression that matches the entire error message.

PR-URL: nodejs#12139
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
@aqrln @MylesBorins
doc: add aqrln to collaborators
72e011e 
Add Alexey Orlenko (@aqrln) to the list of collaborators in README.md

PR-URL: nodejs#12273
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@gibfahn @MylesBorins
doc: document the performance team
c520aef 
Can be cc'ed with `@nodejs/performance`.

PR-URL: nodejs#12213
Refs: nodejs#12194 (comment)
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@vsemozhetbyt @MylesBorins
doc: fix confusing reference in net.md
6577f3e 
PR-URL: nodejs#12247
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
@MylesBorins
doc: update information on test/known_issues
663a1ec 
* Since nodejs#6559 known_issues
  does run on CI.
* Add some notes to explain the expectations around tests in
  known_issues.

PR-URL: nodejs#12262
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@vsemozhetbyt @MylesBorins
doc: fix confusing example in process.md
4bfa2bb 
PR-URL: nodejs#12282
Fixes: nodejs#12280
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@Trott @MylesBorins
tools: ignore URLs in line length linting
f463c17 
Where inclusion of a lengthy URL causes a line to exceed 80 characters
in our code base, do not report the line length as a linting error.

PR-URL: nodejs#11890
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@richardlau @MylesBorins
doc: add richardlau to collaborators
52511b6 
PR-URL: nodejs#12020
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@refack @MylesBorins
doc: add refack to collaborators
97097af 
PR-URL: nodejs#12277
Refs: https://github.com/nodejs/node/blob/master/doc/onboarding.md
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
@danbev @MylesBorins
deps: backport ec1ffe3 from upstream V8
6e8419e 
This commit adds lldbinit files from upstream V8 and also adds these so
that they get installed when `make install` is run.

Original commit message:

[tools] add lldbinit

    The goal of this commit is to add the equivalent to gdbinit but
    for lldb. I've tried to replicate the commands as close as possible
    but I'm unsure about the jss command and hoping to get some feedback
    on it in addition to the bta command which I'm not sure how/when
    this could be used. This is probably just inexperience on my part.

    The lldbinit file can be placed into a directory prefixed with dot
    (.lldbinit) and the python script is currently expected to be in the
    same directory. The path to the script can be changed manually if
    needed as well.

    NOTRY=true

    Review-Url: https://codereview.chromium.org/2758373002
    Cr-Commit-Position: refs/heads/master@{nodejs#44136}

PR-URL: nodejs#12061
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@bnoordhuis
Revert "v8: drop v8::FunctionCallbackInfo<T>::NewTarget()"
4976e13 
See the commit log of the reverted commit: it's a semver-minor change
that can land in the next minor release.

This reverts commit 47cbb88.
@bnoordhuis
Copy link
Member Author

Rebased, /cc @MylesBorins.

Node CI: https://ci.nodejs.org/job/node-test-pull-request/7509/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/657/

@MylesBorins
Copy link
Contributor

landed in c6060fa

@bnoordhuis bnoordhuis mentioned this pull request Sep 29, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees
No one assigned
Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.