★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/25230Nouvelle recherche | Portail wanayoo
Skip to content

/node

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 warning in cares_wrap.cc #25230

Merged
merged 1 commit into from Dec 31, 2018

Conversation

Reviewers

@bnoordhuis bnoordhuis

@jasnell jasnell

@addaleax addaleax

@lpinca lpinca

@BridgeAR BridgeAR

Assignees
No one assigned
Labels
C++ author ready cares
Projects
None yet
Milestone
No milestone
7 participants
@cjihrig @nodejs-github-bot @addaleax @bnoordhuis @jasnell @lpinca @BridgeAR
@cjihrig
Copy link
Contributor

commented Dec 26, 2018

This commit fixes the following warning:

./src/cares_wrap.cc:1268:5: warning: comparison of integers of
    different signs: 'uint32_t' (aka 'unsigned int') and 'int'
    [-Wsign-compare]
    CHECK_EQ(ret->Length(), a_count + aaaa_count);
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added C++ cares labels Dec 26, 2018

@lpinca

lpinca approved these changes Dec 26, 2018
View changes

@addaleax

addaleax approved these changes Dec 26, 2018
View changes

@BridgeAR

BridgeAR approved these changes Dec 27, 2018
View changes

@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 27, 2018

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

@addaleax addaleax added the author ready label Dec 27, 2018

@jasnell

jasnell approved these changes Dec 27, 2018
View changes

@bnoordhuis

bnoordhuis reviewed Dec 28, 2018
View changes

Copy link
Member

left a comment

I'd change a_count and aaaa_count to uint32_t. They're assigned/derived from ret->Length() so they shouldn't be signed in the first place.

@cjihrig cjihrig force-pushed the cjihrig:warning2 branch from 86f28a8 to 0afbbd2 Dec 28, 2018

@cjihrig

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Dec 28, 2018

Yellow resume CI: https://ci.nodejs.org/job/node-test-pull-request/19851/, so this can land as is.

I also pushed up a second commit with @bnoordhuis suggestion instead. The diff got bigger, but @bnoordhuis what do you think?
@addaleax

addaleax approved these changes Dec 30, 2018
View changes

Copy link
Member

left a comment

👍
@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 31, 2018

Fresh CI for the current variant: https://ci.nodejs.org/job/node-test-pull-request/19883/
@cjihrig

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Dec 31, 2018

Yellow resume CI: https://ci.nodejs.org/job/node-test-pull-request/19896/

@cjihrig cjihrig force-pushed the cjihrig:warning2 branch from 0afbbd2 to e18ca7f Dec 31, 2018

@cjihrig
src: fix warning in cares_wrap.cc 
This commit fixes the following warning:

./src/cares_wrap.cc:1268:5: warning: comparison of integers of
    different signs: 'uint32_t' (aka 'unsigned int') and 'int'
    [-Wsign-compare]
    CHECK_EQ(ret->Length(), a_count + aaaa_count);

PR-URL: #25230
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Verified
This commit was signed with a verified signature.
@cjihrig cjihrig Colin Ihrig
GPG key ID: 7434390BDBE9B9C5 Learn about signing commits
Loading status checks…
da9a4d0

@cjihrig cjihrig force-pushed the cjihrig:warning2 branch from e18ca7f to da9a4d0 Dec 31, 2018

@cjihrig cjihrig merged commit da9a4d0 into nodejs:master Dec 31, 2018

@cjihrig cjihrig deleted the cjihrig:warning2 branch Dec 31, 2018

targos added a commit that referenced this pull request Jan 1, 2019

@cjihrig @targos
src: fix warning in cares_wrap.cc 
This commit fixes the following warning:

./src/cares_wrap.cc:1268:5: warning: comparison of integers of
    different signs: 'uint32_t' (aka 'unsigned int') and 'int'
    [-Wsign-compare]
    CHECK_EQ(ret->Length(), a_count + aaaa_count);

PR-URL: #25230
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
Loading status checks…
f62e35f

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

@cjihrig @refack
src: fix warning in cares_wrap.cc 
This commit fixes the following warning:

./src/cares_wrap.cc:1268:5: warning: comparison of integers of
    different signs: 'uint32_t' (aka 'unsigned int') and 'int'
    [-Wsign-compare]
    CHECK_EQ(ret->Length(), a_count + aaaa_count);

PR-URL: nodejs#25230
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
26316e9

@BridgeAR BridgeAR referenced this pull request Jan 16, 2019

Merged

v11.7.0 proposal #25537

@MylesBorins MylesBorins referenced this pull request Jan 24, 2019

Merged

v11.8.0 proposal #25687

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.