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

dtrace: resolve conversion warning (Windows) #10143

Closed
wants to merge 1 commit into from

Conversation

brodycj
Copy link

@brodycj brodycj commented Dec 6, 2016

Checklist
  • make -j8 test (UNIX [macOS]; Linux i386 & amd64) and vcbuild test nosign (.\vcbuild.bat nosign then .\vcbuild.bat test nosign nobuild as Administrator on Windows) PASS OK
  • commit message follows commit guidelines
Additional item
Affected core subsystem(s)

dtrace

Description of change

static_cast<int32_t> in SLURP_INT macro - resolve conversion warning on Windows

NOTE: This was originally a part of PR #10139 (build, warning, header, and include fixes).

ADDITIONAL NOTE: This and some warnings related to PR #10139 may indicate potential data loss scenarios. For future examination.

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 6, 2016
->ToInteger(env->isolate())->Value();
*valp = \
static_cast<int32_t>(obj->Get(OneByteString(env->isolate(), #member)) \
->ToInteger(env->isolate())->Value());
Copy link
Member

Choose a reason for hiding this comment

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

obj->Get(...)->Int32Value()?

Copy link
Author

Choose a reason for hiding this comment

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

+5

@brodycj brodycj force-pushed the cb-dtrace-warning-fix branch 2 times, most recently from 75636a5 to 9087785 Compare December 7, 2016 13:41
@brodycj
Copy link
Author

brodycj commented Dec 7, 2016

@bnoordhuis I did redo the fix according to your suggestion, thanks for the feedback!

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.

LGTM but can you spruce up the commit log a little? I.e., more than just "(Windows)"?

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

@brodycj
Copy link
Author

brodycj commented Dec 7, 2016

Yes the log could have been better. I wanted to put the "(Windows)" part on the subject line but then I couldn't fit a nice, complete commit subject into the standard 50 character length.

My idea is to replace "(Windows)" with "(Windows build)". Short, sweet, and to the point. @bnoordhuis please reply if this is still not quite good enough. Otherwise I will do this tomorrow morning.

@bnoordhuis
Copy link
Member

Take a look at other commits for inspiration. It should consist of a short status line and a few more lines describing the what and why. I usually include the compiler message verbatim when it's a fix for a compiler warning.

@brodycj brodycj force-pushed the cb-dtrace-warning-fix branch 2 times, most recently from a6de333 to d463558 Compare December 8, 2016 07:47
@brodycj
Copy link
Author

brodycj commented Dec 8, 2016

I just amended the commit with the following message:

dtrace: resolve conversion warnings from SLURP_INT

Resolve build warnings on Windows with the following pattern:
warning C4244: '=': conversion from 'int64_t' to 'int32_t',
possible loss of data

Resolve build warnings on Windows with the following pattern:
warning C4244: '=': conversion from 'int64_t' to 'int32_t',
possible loss of data
jasnell pushed a commit that referenced this pull request Dec 27, 2016
Resolve build warnings on Windows with the following pattern:
warning C4244: '=': conversion from 'int64_t' to 'int32_t',
possible loss of data

PR-URL: #10143
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Dec 27, 2016

Landed in 099762d

@jasnell jasnell closed this Dec 27, 2016
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
Resolve build warnings on Windows with the following pattern:
warning C4244: '=': conversion from 'int64_t' to 'int32_t',
possible loss of data

PR-URL: #10143
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
Resolve build warnings on Windows with the following pattern:
warning C4244: '=': conversion from 'int64_t' to 'int32_t',
possible loss of data

PR-URL: #10143
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

Should this be backported?

@MylesBorins
Copy link
Contributor

ping

@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 17, 2017

Should this be backported?

ping @bnoordhuis I guess

@bnoordhuis
Copy link
Member

Could be back-ported.

gibfahn pushed a commit that referenced this pull request Jun 20, 2017
Resolve build warnings on Windows with the following pattern:
warning C4244: '=': conversion from 'int64_t' to 'int32_t',
possible loss of data

PR-URL: #10143
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
Resolve build warnings on Windows with the following pattern:
warning C4244: '=': conversion from 'int64_t' to 'int32_t',
possible loss of data

PR-URL: #10143
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
Resolve build warnings on Windows with the following pattern:
warning C4244: '=': conversion from 'int64_t' to 'int32_t',
possible loss of data

PR-URL: #10143
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
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.

6 participants