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

src: fix CreatePlatform header param mismatch #23947

Closed
wants to merge 1 commit into from
Closed

src: fix CreatePlatform header param mismatch #23947

wants to merge 1 commit into from

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Oct 29, 2018

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 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
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

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

@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

@danbev
Copy link
Contributor

danbev commented Nov 2, 2018

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
PR-URL: #23947
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
targos pushed a commit that referenced this pull request Nov 2, 2018
PR-URL: #23947
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
ofrobots pushed a commit to ofrobots/node that referenced this pull request Nov 8, 2018
PR-URL: nodejs#23947
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 12, 2018
Backport-PR-URL: #23700
PR-URL: #23947
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Backport-PR-URL: #23700
PR-URL: #23947
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Backport-PR-URL: #23700
PR-URL: #23947
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
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++. embedding Issues and PRs related to embedding Node.js in another project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants