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: turn buffer type-CHECK into exception #12753

Closed
wants to merge 2 commits into from
Closed

src: turn buffer type-CHECK into exception #12753

wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

Turn a CHECK() that could be brought to fail using public APIs
into throwing an error.

Fixes: #12152

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

stream_base

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 29, 2017
@addaleax addaleax mentioned this pull request Apr 29, 2017
Closed
@mscdex mscdex added the stream Issues and PRs related to the stream subsystem. label Apr 30, 2017
@addaleax addaleax added lib / src Issues and PRs related to general changes in the lib or src directory. and removed stream Issues and PRs related to the stream subsystem. labels May 3, 2017
@addaleax
Copy link
Member Author

addaleax commented May 3, 2017

CI: https://ci.nodejs.org/job/node-test-commit/9601/

addaleax added 2 commits May 3, 2017 15:43
@addaleax
src: turn buffer type-CHECK into exception
6ef674f 
Turn a `CHECK()` that could be brought to fail using public APIs
into throwing an error.

Fixes: nodejs#12152
@addaleax
[squash] fixup test
a9e1bba
@addaleax addaleax force-pushed the fix-type-confusion-cash-12152 branch from 753a650 to a9e1bba Compare May 3, 2017 13:47
@addaleax
Copy link
Member Author

addaleax commented May 3, 2017

Had to fix up the test because it previously depended on the type of process.stdout, PTAL

New CI: https://ci.nodejs.org/job/node-test-commit/9603/

@refack
Copy link
Contributor

refack commented May 3, 2017 •
edited
Loading

It's a test, it's run on it's own, who can manipulate process.stdout?
Figured out it's tap...

@addaleax
Copy link
Member Author

addaleax commented May 3, 2017

Figured out it's tap...

tap? ^^ It’s our test runner that runs it with a temporary file as stdout, yea.

@addaleax
Copy link
Member Author

addaleax commented May 3, 2017

Landed in 9990be2

@addaleax addaleax closed this May 3, 2017
@addaleax addaleax deleted the fix-type-confusion-cash-12152 branch May 3, 2017 17:17
addaleax added a commit that referenced this pull request May 3, 2017
@addaleax
src: turn buffer type-CHECK into exception
9990be2 
Turn a `CHECK()` that could be brought to fail using public APIs
into throwing an error.

Fixes: #12152
PR-URL: #12753
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
@addaleax
src: turn buffer type-CHECK into exception
d523103 
Turn a `CHECK()` that could be brought to fail using public APIs
into throwing an error.

Fixes: nodejs#12152
PR-URL: nodejs#12753
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@jasnell jasnell mentioned this pull request May 11, 2017
Closed
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

@addaleax should this be backported?

@addaleax
Copy link
Member Author

@gibfahn Yes.

gibfahn pushed a commit that referenced this pull request Jun 18, 2017
@addaleax @gibfahn
src: turn buffer type-CHECK into exception
ae38adc 
Turn a `CHECK()` that could be brought to fail using public APIs
into throwing an error.

Fixes: #12152
PR-URL: #12753
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
@addaleax @gibfahn
src: turn buffer type-CHECK into exception
a0d377b 
Turn a `CHECK()` that could be brought to fail using public APIs
into throwing an error.

Fixes: #12152
PR-URL: #12753
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
@addaleax @MylesBorins
src: turn buffer type-CHECK into exception
f43c969 
Turn a `CHECK()` that could be brought to fail using public APIs
into throwing an error.

Fixes: #12152
PR-URL: #12753
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
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
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@addaleax @refack @gibfahn @jasnell @TimothyGu @cjihrig @mscdex @nodejs-github-bot