FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: fix multiple AddLinkedBinding() calls by addaleax · Pull Request #39012 · nodejs/node · GitHub

/ node Public

src: fix multiple AddLinkedBinding() calls - #39012

Closed
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:many-linked-bindings
Closed

src: fix multiple AddLinkedBinding() calls#39012
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:many-linked-bindings

Conversation

Copy link
Copy Markdown
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 added embedding Issues and PRs related to embedding Node.js in another project. lts-watch-v12.x labels Jun 11, 2021
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 removed the needs-ci PRs that need a full CI run. label Jun 11, 2021

Copy link
Copy Markdown
Collaborator

addaleax added the review wanted PRs that need reviews. label Jun 13, 2021

Copy link
Copy Markdown
Member Author

/cc @nodejs/embedders

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

addaleax requested a review from targos June 14, 2021 09:21

targos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
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 👍

Copy link
Copy Markdown
Member Author

Landed in cd43073

addaleax closed this Jun 14, 2021
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 removed lts-watch-v14.x review wanted PRs that need reviews. labels Jun 14, 2021

targos commented Jun 14, 2021

Copy link
Copy Markdown
Member

Added to #38948

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>

Copy link
Copy Markdown
Member

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL