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

zlib: finish migrating to internal/errors #16540

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Oct 27, 2017

Finish migrating the zlib binding over to use internal/errors

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

zlib, errors

@jasnell jasnell added errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version. zlib Issues and PRs related to the zlib subsystem. labels Oct 27, 2017
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. errors Issues and PRs related to JavaScript errors originated in Node.js core. zlib Issues and PRs related to the zlib subsystem. labels Oct 27, 2017
@jasnell
Copy link
Member Author

jasnell commented Oct 27, 2017

ping @nodejs/tsc

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.

FWIW

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.

LGTM with a question?

write_js_callback, dictionary, dictionary_len);
bool ret = Init(ctx, level, windowBits, memLevel, strategy, write_result,
write_js_callback, dictionary, dictionary_len);
if (!ret) goto end;
Copy link
Member

Choose a reason for hiding this comment

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

Any reason we need a goto here? (other than one less line of duplicate code?)

Copy link
Member Author

Choose a reason for hiding this comment

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

just that... didn't want to duplicate the line of code

@joyeecheung
Copy link
Member

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

common.expectsError(
() => zlib.createDeflateRaw({ windowBits: 8 }),
{
code: 'ERR_ZLIB_INITIALIZATION_FAILED',
Copy link
Member

Choose a reason for hiding this comment

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

We need a new way to test this error once #16511 is merged.

@jasnell jasnell force-pushed the migrate-zlib-internal-errors branch from 0257a1d to c8ef9c3 Compare October 29, 2017 17:15
jasnell added a commit that referenced this pull request Oct 29, 2017
PR-URL: #16540
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Oct 29, 2017

Landed in 896eaf6

@jasnell jasnell closed this Oct 29, 2017
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
PR-URL: nodejs/node#16540
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
PR-URL: nodejs/node#16540
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
PR-URL: nodejs/node#16540
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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++. errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants