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 multiple AddLinkedBinding() calls #39012

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

Singly-linked lists are extended at their tail, not their head.
This fixes using more than 2 linked addons at a time.


I would appreciate early backports to v14.x and v12.x, since this is actually something I’m running into at work, and it’s a) a bugfix and b) only affects embedding scenarios, which makes it really low-risk for LTS imo.

Singly-linked lists are extended at their tail, not their head.
This fixes using more than 2 linked addons at a time.
@addaleax addaleax added embedding Issues and PRs related to embedding Node.js in another project. lts-watch-v12.x labels Jun 11, 2021
@github-actions github-actions bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jun 11, 2021
@addaleax addaleax removed the needs-ci PRs that need a full CI run. label Jun 11, 2021
@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the review wanted PRs that need reviews. label Jun 13, 2021
@addaleax
Copy link
Member Author

/cc @nodejs/embedders

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax requested a review from targos June 14, 2021 09:21
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

RSLGTM. Did you ask for my review because you would like it to be cherry-picked to #38948 ?

@addaleax
Copy link
Member Author

@targos I just thought you’d be a good person here to take a look – if you can and want to do that, that would of course make things easier for me, but next release is also fine 👍

@addaleax
Copy link
Member Author

Landed in cd43073

@addaleax addaleax closed this Jun 14, 2021
@addaleax addaleax deleted the many-linked-bindings branch June 14, 2021 11:31
addaleax added a commit that referenced this pull request Jun 14, 2021
Singly-linked lists are extended at their tail, not their head.
This fixes using more than 2 linked addons at a time.

PR-URL: #39012
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
targos pushed a commit that referenced this pull request Jun 14, 2021
Singly-linked lists are extended at their tail, not their head.
This fixes using more than 2 linked addons at a time.

PR-URL: #39012
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
targos pushed a commit that referenced this pull request Jun 14, 2021
Singly-linked lists are extended at their tail, not their head.
This fixes using more than 2 linked addons at a time.

PR-URL: #39012
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
@targos targos removed lts-watch-v14.x review wanted PRs that need reviews. labels Jun 14, 2021
@targos
Copy link
Member

targos commented Jun 14, 2021

Added to #38948

@danielleadams danielleadams mentioned this pull request Jun 14, 2021
danielleadams pushed a commit that referenced this pull request Jun 17, 2021
Singly-linked lists are extended at their tail, not their head.
This fixes using more than 2 linked addons at a time.

PR-URL: #39012
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
@richardlau
Copy link
Member

I think this depends on #35301 which isn't on v12.x-staging.

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.

5 participants