Skip to content

src: fix warning in cares_wrap.cc#25230

Merged
cjihrig merged 1 commit intonodejs:masterfrom
cjihrig:warning2
Dec 31, 2018
Merged

src: fix warning in cares_wrap.cc#25230
cjihrig merged 1 commit intonodejs:masterfrom
cjihrig:warning2

Conversation

@cjihrig
Copy link
Contributor

@cjihrig cjihrig 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
Copy link
Collaborator

nodejs-github-bot commented Dec 26, 2018

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. labels Dec 26, 2018
@addaleax
Copy link
Member

addaleax commented Dec 27, 2018

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 27, 2018
bnoordhuis
bnoordhuis reviewed Dec 28, 2018
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

cjihrig 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 reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

👍

@addaleax
Copy link
Member

addaleax commented Dec 31, 2018

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

@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 31, 2018

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

@cjihrig
src: fix warning in cares_wrap.cc
da9a4d0 
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig cjihrig merged commit da9a4d0 into nodejs:master Dec 31, 2018
@cjihrig cjihrig deleted the warning2 branch December 31, 2018 16:42
targos pushed a commit that referenced this pull request Jan 1, 2019
@cjihrig @targos
src: fix warning in cares_wrap.cc
f62e35f 
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@cjihrig @refack
src: fix warning in cares_wrap.cc
26316e9 
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
Merged
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
Merged
BethGriggs pushed a commit that referenced this pull request Jul 2, 2019
@cjihrig @BethGriggs
src: fix warning in cares_wrap.cc
cabcc14 
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Jul 17, 2019
@cjihrig @BethGriggs
src: fix warning in cares_wrap.cc
b7dbc1c 
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Jul 17, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis left review comments

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca 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++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@cjihrig @nodejs-github-bot @addaleax @bnoordhuis @jasnell @lpinca @BridgeAR @BethGriggs