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: move AsyncResource impl out of public header #26348

Closed
wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Feb 28, 2019

Implementing the methods out-of-line (i.e., not inline) means we can fix
bugs and have already compiled add-ons pick up the fixes automatically,
something that doesn't work when the methods are inline because then
they get compiled into the add-on instead of the node binary.

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

@nodejs-github-bot
Copy link
Collaborator

@bnoordhuis sadly an error occured when I tried to trigger a build :(

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. labels Feb 28, 2019
@addaleax
Copy link
Member

I don’t quite understand the Windows CI failure (there doesn’t seem to be information about where in our source it is coming from), but it seems related.

@bnoordhuis
Copy link
Member Author

Rebase + new CI to check if it's actually a persistent issue: https://ci.nodejs.org/job/node-test-pull-request/21086/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 1, 2019
@refack
Copy link
Contributor

refack commented Mar 1, 2019

Windows compile fail is triggered by the NODE_EXTERN from line
https://github.com/nodejs/node/blob/a3dce1b5e115e7c08c2f3e2e62ede6ba3c1d93d5/src/node.h#L703
by line
https://github.com/nodejs/node/blob/a3dce1b5e115e7c08c2f3e2e62ede6ba3c1d93d5/src/node.h#L739

D:\code\prws\deps\v8\include\v8.h(631): error C2440: '=': cannot convert from 'v8::Primitive *' to 'O *volatile ' [D:\code\prws\node_lib.vcxproj]
          with
          [
              O=v8::Object
          ]
  D:\code\prws\deps\v8\include\v8.h(631): note: Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast
  D:\code\prws\deps\v8\include\v8.h(627): note: see reference to function template instantiation 'void v8::NonCopyablePersistentTraits<T>::Uncompilable<v8::Object>(void)' being compiled
          with
          [
              T=v8::Object
          ]

src/node.h Show resolved Hide resolved
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 2, 2019
@BridgeAR
Copy link
Member

BridgeAR commented Mar 5, 2019

Ping @bnoordhuis

Implementing the methods out-of-line (i.e., not inline) means we can fix
bugs and have already compiled add-ons pick up the fixes automatically,
something that doesn't work when the methods are inline because then
they get compiled into the add-on instead of the node binary.
@bnoordhuis
Copy link
Member Author

Sorry, missed @refack's comment. Feedback incorporated. New CI: https://ci.nodejs.org/job/node-test-pull-request/21781/

@BridgeAR
Copy link
Member

@refack
Copy link
Contributor

refack commented Mar 22, 2019

Seems like it does compile on Windows now. There's just one test failing that might be related:

Probably it's just being flaky #26798

Resume: https://ci.nodejs.org/job/node-test-pull-request/21814

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 25, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Apr 4, 2019
Implementing the methods out-of-line (i.e., not inline) means we can fix
bugs and have already compiled add-ons pick up the fixes automatically,
something that doesn't work when the methods are inline because then
they get compiled into the add-on instead of the node binary.

PR-URL: nodejs#26348
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@BridgeAR
Copy link
Member

BridgeAR commented Apr 4, 2019

Landed in eb2dccb 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants