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: fix compiler warning for debug build #23994

Closed
wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Oct 31, 2018

This commit updates the check of the offset argument passed to
CallJSOnreadMethod to avoid doing a cast as this currently generates
the following compiler warning when doing a debug build:

./src/stream_base.cc:295:3:
warning: comparison of integers of different signs:
'int32_t' (aka 'int') and 'size_t' (aka 'unsigned long')
[-Wsign-compare]
  CHECK_EQ(static_cast<int32_t>(offset), offset);
  ^        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~  ~~~~~~
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 31, 2018
@addaleax
Copy link
Member

(Fwiw, we usually keep stream: as a subsystem for the userland/JS streams module… I’ve been using src: for all or most StreamBase changes)

This commit updates the check of the offset argument passed to
CallJSOnreadMethod to avoid doing a cast as this currently generates
the following compiler warning when doing a debug build:

./src/stream_base.cc:295:3:
warning: comparison of integers of different signs:
'int32_t' (aka 'int') and 'size_t' (aka 'unsigned long')
[-Wsign-compare]
  CHECK_EQ(static_cast<int32_t>(offset), offset);
  ^        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~  ~~~~~~
@danbev danbev changed the title stream: fix compiler warning for debug build src: fix compiler warning for debug build Nov 1, 2018
@danbev
Copy link
Contributor Author

danbev commented Nov 1, 2018

Trott pushed a commit to Trott/io.js that referenced this pull request Nov 3, 2018
This commit updates the check of the offset argument passed to
CallJSOnreadMethod to avoid doing a cast as this currently generates
the following compiler warning when doing a debug build:

./src/stream_base.cc:295:3:
warning: comparison of integers of different signs:
'int32_t' (aka 'int') and 'size_t' (aka 'unsigned long')
[-Wsign-compare]
  CHECK_EQ(static_cast<int32_t>(offset), offset);
  ^        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~  ~~~~~~

PR-URL: nodejs#23994
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member

Trott commented Nov 3, 2018

Landed in 2ea70ea

@Trott Trott closed this Nov 3, 2018
targos pushed a commit that referenced this pull request Nov 3, 2018
This commit updates the check of the offset argument passed to
CallJSOnreadMethod to avoid doing a cast as this currently generates
the following compiler warning when doing a debug build:

./src/stream_base.cc:295:3:
warning: comparison of integers of different signs:
'int32_t' (aka 'int') and 'size_t' (aka 'unsigned long')
[-Wsign-compare]
  CHECK_EQ(static_cast<int32_t>(offset), offset);
  ^        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~  ~~~~~~

PR-URL: #23994
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danbev danbev deleted the stream_base_debug_warning branch November 5, 2018 05:22
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++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants