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.

Sign up for GitHub

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

Jump to bottom

src: use default parameters for UVException() #23176

Closed
wants to merge 1 commit into from
Closed

src: use default parameters for UVException() #23176

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

(dont-land-on-v10.x because of the ABI change.)

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

@nodejs-github-bot
Copy link
Collaborator

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

@addaleax addaleax added lib / src Issues and PRs related to general changes in the lib or src directory. dont-land-on-v8.x labels Sep 30, 2018
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 30, 2018
joyeecheung
joyeecheung approved these changes Sep 30, 2018
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

I am actually surprised that this is exported..is there any modules actually using this? I couldn't find anything when I did a quick search on GitHub.

@joyeecheung
Copy link
Member

joyeecheung commented Sep 30, 2018 •
edited
Loading

Oh nvm, just saw #23175

@addaleax
Copy link
Member Author

@joyeecheung I’m not sure, but of the many, many things in node.h this seems like something that could actually be useful to addons…

@joyeecheung
Copy link
Member

I’m not sure, but of the many, many things in node.h this seems like something that could actually be useful to addons…

@addaleax I can see what you mean...it probably makes sense to add something like this to N-API? (Not sure about how comfortable we are with exposing libuv related stuff there but we do have napi_get_uv_event_loop anyway)

@addaleax
Copy link
Member Author

I guess that would be part of the larger libuv/N-API discussion (libuv/libuv#1931) … I’m still personally inclined to let libuv be and mark a subset of it as ABI-stable, which would mean that we don’t need to do anything ;)

@targos targos added this to Don't land (ever) in v10.x Oct 3, 2018
@addaleax
Copy link
Member Author

addaleax commented Oct 3, 2018

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 3, 2018
@danbev
Copy link
Contributor

danbev commented Oct 4, 2018

Landed in 51f0060.

@danbev danbev closed this Oct 4, 2018
v10.x automation moved this from Don't land (ever) to Closed PRs Oct 4, 2018
danbev pushed a commit that referenced this pull request Oct 4, 2018
@addaleax @danbev
src: use default parameters for UVException()
51f0060 
PR-URL: #23176
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@targos targos moved this from Closed PRs to Don't land (ever) in v10.x Oct 5, 2018
jasnell pushed a commit that referenced this pull request Oct 17, 2018
@addaleax @jasnell
src: use default parameters for UVException()
8f84613 
PR-URL: #23176
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
No open projects
v10.x
  
Don't land (ever)
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@addaleax @nodejs-github-bot @joyeecheung @danbev @refack @jasnell @cjihrig @BridgeAR