src: fix compiler warning in udp_wrap.cc by danbev · Pull Request #15402 · nodejs/node · GitHub

Currently the following compiler warning is generated:

1 warning generated.
../src/udp_wrap.cc:238:12: warning: comparison of integers of different
signs: 'int' and 'uint32_t' (aka 'unsigned int') [-Wsign-compare]
  if (size != args[0].As<Uint32>()->Value()) {
      ~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

This commit changes the check to see that the Uint32 value does not
exceed the max int size instead of first casting and then comparing.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

src

Better to use uint32_t?

Much better, will update now. Thanks!

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

Updated CI run: https://ci.nodejs.org/job/node-test-pull-request/10073/

shouldn't this be static_cast<uint32_t> ?

actually, Uint32::Value() returns a uint32_t already so you can remove the cast

@targos Thanks!

I was just focusing on the compiler warning and not giving proper attention to the code I'm afraid. I think it was correct to have the cast to int as this is then checked to make sure that the value can fit into an int before passing it along to uv_recv_buffer_size or uv_send_buffer_size. Otherwise it will never detect such a situation.

Thanks!

There is a IsInt32() function, https://cs.chromium.org/chromium/src/v8/src/api.cc?q=IsUint32&sq=package:chromium&l=3694.

If you want, we could replace the > check with IsInt32(). But I'm also fine with landing as is, as it's correct and gets rid of the compiler warning.

There is a bit too much typecasting to my taste. I'd probably assign it to a uint32_t, do the range check and only then assign it to an int. IsInt32() would work too since the CHECK(args[0]->IsUint32()) a few lines up establishes that it's >= 0.

Aside: the second argument is really just a bool in disguise. The logic in lib/dgram.js could be simplified to self._handle.bufferSize(size, buffer === 'recv') if the code here was updated to expect a bool.

Aside aside: since this is new code, it should have been written to use the Value() overloads that take a Local<Context>. Oh well.

@fhinkel Nice, I was not aware of that. I'll update the PR. Thanks!

@bnoordhuis I'd be happy to take a closer look at this, but perhaps as a separate PR?

Currently the following compiler warning is generated: 1 warning generated. ../src/udp_wrap.cc:238:12: warning: comparison of integers of different signs: 'int' and 'uint32_t' (aka 'unsigned int') [-Wsign-compare] if (size != args[0].As<Uint32>()->Value()) { ~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. This commit changes the check to see that the Uint32 value does not exceed the max int size instead of first casting and then comparing.

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

test/aix failure looks unrelated

console output:

not ok 1686 inspector/test-stop-profile-after-done # TODO : Fix flaky test
  ---
  duration_ms: 0.791
  severity: flaky
  stack: |-
    [test] Connecting to a child Node process
    [test] Testing /json/list
    [err] Debugger listening on ws://127.0.0.1:55055/85359bfe-2a33-4295-936c-616bbf936f31
    [err] For help see https://nodejs.org/en/docs/inspector
    [err] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    [out] {}
    [out] 
    { Error: connect ECONNREFUSED 127.0.0.1:55055
        at Object._errnoException (util.js:1018:13)
        at _exceptionWithHostPort (util.js:1039:20)
        at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1179:14)
      errno: 'ECONNREFUSED',
      code: 'ECONNREFUSED',
      syscall: 'connect',
      address: '127.0.0.1',
      port: 55055 }
    1
  ...

test/arm-fanned failure looks unrelated

console output:

+ git clean -fdx
warning: failed to remove out/
Removing out/
Build step 'Execute shell' marked build as failure
TAP Reports Processing: START
Looking for TAP results report in workspace using pattern: *.tap
Did not find any matching files. Setting build result to FAILURE.
Checking ^not ok
Jenkins Text Finder: File set '*.tap' is empty
Notifying upstream projects of job completion
Finished: FAILURE

Currently the following compiler warning is generated: 1 warning generated. ../src/udp_wrap.cc:238:12: warning: comparison of integers of different signs: 'int' and 'uint32_t' (aka 'unsigned int') [-Wsign-compare] if (size != args[0].As<Uint32>()->Value()) { ~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. This commit changes the check to see that the Uint32 value does not exceed the max int size instead of first casting and then comparing. PR-URL: nodejs#15402 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

Landed in 87bddda.

@targos @fhinkel Thanks!

This does not land cleanly in v8.x as it depends on #13623 and should be backported along with that one.

Currently the following compiler warning is generated: 1 warning generated. ../src/udp_wrap.cc:238:12: warning: comparison of integers of different signs: 'int' and 'uint32_t' (aka 'unsigned int') [-Wsign-compare] if (size != args[0].As<Uint32>()->Value()) { ~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. This commit changes the check to see that the Uint32 value does not exceed the max int size instead of first casting and then comparing. PR-URL: nodejs/node#15402 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

Currently the following compiler warning is generated: 1 warning generated. ../src/udp_wrap.cc:238:12: warning: comparison of integers of different signs: 'int' and 'uint32_t' (aka 'unsigned int') [-Wsign-compare] if (size != args[0].As<Uint32>()->Value()) { ~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. This commit changes the check to see that the Uint32 value does not exceed the max int size instead of first casting and then comparing. PR-URL: #15402 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

setting as dont-land-v6.x as the commit this is based on is Semver-Minor and has not landed in v6.x. Please lmk if you think we should reconsider

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dgram Issues and PRs related to the dgram subsystem / UDP. labels Sep 14, 2017

mscdex reviewed Sep 14, 2017

View reviewed changes

danbev force-pushed the udp_wrap_compiler_warning branch from 282baf3 to c91f5ec Compare September 14, 2017 06:52

danbev force-pushed the udp_wrap_compiler_warning branch from c91f5ec to a1c2c81 Compare September 14, 2017 07:34

targos reviewed Sep 14, 2017

View reviewed changes

danbev force-pushed the udp_wrap_compiler_warning branch from a1c2c81 to 115989a Compare September 15, 2017 05:21

fhinkel reviewed Sep 16, 2017

View reviewed changes

fhinkel approved these changes Sep 16, 2017

View reviewed changes

danbev force-pushed the udp_wrap_compiler_warning branch from 115989a to ff211e9 Compare September 17, 2017 10:06

danbev closed this Sep 20, 2017

danbev deleted the udp_wrap_compiler_warning branch September 20, 2017 14:00

jasnell mentioned this pull request Sep 20, 2017

Dgram buffers #13623

Closed

4 tasks

jasnell added the backport-requested-v8.x label Sep 20, 2017

MylesBorins removed the backport-requested-v8.x label Sep 28, 2017

MylesBorins mentioned this pull request Oct 3, 2017

v8.7.0 proposal #15762

Merged

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

src: fix compiler warning in udp_wrap.cc by danbev · Pull Request #15402 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: fix compiler warning in udp_wrap.cc - #15402

src: fix compiler warning in udp_wrap.cc#15402danbev wants to merge 1 commit intonodejs:masternodejs/node:masterfrom danbev:udp_wrap_compiler_warningdanbev/node:udp_wrap_compiler_warningCopy head branch name to clipboard

Conversation

danbev commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbev commented Sep 14, 2017

Uh oh!

danbev commented Sep 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbev commented Sep 20, 2017

Uh oh!

danbev commented Sep 20, 2017

Uh oh!

danbev commented Sep 20, 2017

Uh oh!

jasnell commented Sep 20, 2017

Uh oh!

MylesBorins commented Oct 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

src: fix compiler warning in udp_wrap.cc#15402
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:udp_wrap_compiler_warning

danbev commented Sep 14, 2017 •
edited
Loading