Skip to content

src: fix CreatePlatform header param mismatch#23947

Closed
codebytere wants to merge 1 commit intonodejs:masterfrom
codebytere:fix-header
Closed

src: fix CreatePlatform header param mismatch#23947
codebytere wants to merge 1 commit intonodejs:masterfrom
codebytere:fix-header

Conversation

@codebytere
Copy link
Member

@codebytere codebytere commented Oct 29, 2018 •
edited
Loading

This PR forward-declares node::tracing::TracingController instead of v8::TracingController to correct for the existing mismatch in parameters between MultiIsolatePlatform CreatePlatform in the header and impl. It can cause inconsistencies in exported symbols.

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 29, 2018

@codebytere build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1405/pipeline

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 29, 2018
@codebytere codebytere added the embedding Issues and PRs related to embedding Node.js in another project. label Oct 29, 2018
@refack refack added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Oct 29, 2018
refack
refack approved these changes Oct 29, 2018
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

LGTM

@refack
Copy link
Contributor

refack commented Oct 29, 2018

At first glance it seems semver-major, but IIUC this signature could never have been linked against, since there is no definition for a

NODE_EXTERN MultiIsolatePlatform* CreatePlatform(
    int thread_pool_size,
    v8::TracingController* tracing_controller)

function in the code.
So I'm +1 for considering this semver-patch.

@addaleax
Copy link
Member

addaleax commented Oct 29, 2018

At first glance it seems semver-major, but IIUC this signature could never have been linked against, since there is no definition for a

So … for context, this was broken in f5986a4, meaning that this patch (as it is) would currently only apply on v11.x. There is a backport PR of that change open for v10.x.

/cc @ofrobots

@addaleax addaleax mentioned this pull request Oct 29, 2018
Closed
4 tasks
@refack
Copy link
Contributor

refack commented Oct 29, 2018

So … for context, this was broken in f5986a4,

Reminds me of my old dream to have a regression test for the embedding "API"...

@danbev
Copy link
Contributor

danbev commented Nov 2, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/18292/

@danbev
Copy link
Contributor

danbev commented Nov 2, 2018 •
edited
Loading

Re-run of failing node-test-commit-windows-fanned ✔️

@danbev
Copy link
Contributor

danbev commented Nov 2, 2018

Landed in 1d5007b.

@danbev danbev closed this Nov 2, 2018
danbev pushed a commit that referenced this pull request Nov 2, 2018
@codebytere @danbev
src: fix CreatePlatform header param mismatch
1d5007b 
PR-URL: #23947
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
targos pushed a commit that referenced this pull request Nov 2, 2018
@codebytere @targos
src: fix CreatePlatform header param mismatch
84e5807 
PR-URL: #23947
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
ofrobots pushed a commit to ofrobots/node that referenced this pull request Nov 8, 2018
@codebytere @ofrobots
src: fix CreatePlatform header param mismatch
b65e585 
PR-URL: nodejs#23947
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
MylesBorins pushed a commit that referenced this pull request Nov 12, 2018
@codebytere @MylesBorins
src: fix CreatePlatform header param mismatch
9a7554a 
Backport-PR-URL: #23700
PR-URL: #23947
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@BridgeAR BridgeAR mentioned this pull request Nov 14, 2018
Merged
This was referenced Nov 15, 2018
Closed
Closed
Closed
Closed
This was referenced Nov 16, 2018
Closed
Closed
@codebytere codebytere mentioned this pull request Nov 27, 2018
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@codebytere @rvagg
src: fix CreatePlatform header param mismatch
d70b314 
Backport-PR-URL: #23700
PR-URL: #23947
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
@codebytere @MylesBorins
src: fix CreatePlatform header param mismatch
6bc66bf 
Backport-PR-URL: #23700
PR-URL: #23947
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Merged
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

+3 more reviewers

@ofrobots ofrobots ofrobots approved these changes

@refack refack refack approved these changes

@mmarchini mmarchini mmarchini approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. embedding Issues and PRs related to embedding Node.js in another project.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@codebytere @nodejs-github-bot @refack @addaleax @danbev @ofrobots @jasnell @cjihrig @mmarchini @MylesBorins