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: suppress coverity message #7587

Merged
merged 1 commit into from
Jul 8, 2016
Merged

src: suppress coverity message #7587

merged 1 commit into from
Jul 8, 2016

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jul 7, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

Description of change

Coverity marked a change in 630096b as a constant expression. However, on platforms where sizeof(int64_t) > sizeof(size_t), this should not be the case. This commit flags the comparison
as OK to coverity.

Disclaimer: I have no idea how to test this with coverity. I put this together based on existing code in core and some Googling.

R= @bnoordhuis

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

addaleax commented Jul 7, 2016

LGTM

@@ -209,6 +209,8 @@ inline MUST_USE_RESULT bool ParseArrayIndex(Local<Value> arg,

// Check that the result fits in a size_t.
const uint64_t kSizeMax = static_cast<uint64_t>(static_cast<size_t>(-1));
// Tell coverity that this is not a constant expression on all platforms.
Copy link
Member

Choose a reason for hiding this comment

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

It is a constant expression, however. I'd just leave out the comment, it's clear enough from context that we're trying to shut up coverity.

@mscdex mscdex added the buffer Issues and PRs related to the buffer subsystem. label Jul 7, 2016
@jasnell
Copy link
Member

jasnell commented Jul 7, 2016

LGTM with or without the comment

@mhdawson
Copy link
Member

mhdawson commented Jul 7, 2016

LGTM

Coverity marked a change in 630096b as a constant expression.
However, on platforms where sizeof(int64_t) > sizeof(size_t),
this should not be the case. This commit flags the comparison
as OK to coverity.

PR-URL: nodejs#7587
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 8, 2016

Landed in 18ae74c without the extra comment.

@cjihrig cjihrig merged commit 18ae74c into nodejs:master Jul 8, 2016
@cjihrig cjihrig deleted the coverity branch July 8, 2016 16:03
@MylesBorins
Copy link
Contributor

@cjihrig lts?

@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 12, 2016

Yes please.

@MylesBorins
Copy link
Contributor

@cjihrig this does not land cleanly and will require a manual backport if you are up for it

@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 12, 2016

This should apply cleanly once #7497 is backported. If that's not the case, let me know.

@cjihrig cjihrig mentioned this pull request Aug 8, 2016
cjihrig added a commit that referenced this pull request Aug 10, 2016
Coverity marked a change in 630096b as a constant expression.
However, on platforms where sizeof(int64_t) > sizeof(size_t),
this should not be the case. This commit flags the comparison
as OK to coverity.

PR-URL: #7587
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
bnoordhuis pushed a commit to bnoordhuis/io.js that referenced this pull request Oct 19, 2016
Coverity marked a change in 630096b as a constant expression.
However, on platforms where sizeof(int64_t) > sizeof(size_t),
this should not be the case. This commit flags the comparison
as OK to coverity.

PR-URL: nodejs#7587
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Oct 24, 2016
Coverity marked a change in 630096b as a constant expression.
However, on platforms where sizeof(int64_t) > sizeof(size_t),
this should not be the case. This commit flags the comparison
as OK to coverity.

PR-URL: #7587
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
Coverity marked a change in 630096b as a constant expression.
However, on platforms where sizeof(int64_t) > sizeof(size_t),
this should not be the case. This commit flags the comparison
as OK to coverity.

PR-URL: #7587
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. 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.

8 participants