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.

Sign up for GitHub

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

Jump to bottom

src: apply clang-tidy rule bugprone-incorrect-roundings #26885

Merged
merged 1 commit into from
Apr 3, 2019
Merged

src: apply clang-tidy rule bugprone-incorrect-roundings #26885

merged 1 commit into from
Apr 3, 2019

Conversation

gengjiawen
Copy link
Member

see https://clang.llvm.org/extra/clang-tidy/checks/bugprone-incorrect-roundings.html.

Checks the usage of patterns known to produce incorrect rounding. Programmers often use:

(int)(double_expression + 0.5)
to round the double expression to an integer. The problem with this:

It is unnecessarily slow.
It is incorrect. The number 0.499999975 (smallest representable float number below 0.5) rounds to 1.0. Even worse behavior for negative numbers where both -0.5f and -1.4f both round to 0.0.

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

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 24, 2019
mscdex
mscdex reviewed Mar 24, 2019
View reviewed changes
src/node_platform.cc Outdated Show resolved Hide resolved
@gengjiawen gengjiawen force-pushed the bugprone-incorrect-roundings branch from 5b449ce to 9ce6d45 Compare March 24, 2019 14:45
refack
refack suggested changes Mar 25, 2019
View reviewed changes
src/node_platform.cc Outdated Show resolved Hide resolved
@BridgeAR BridgeAR requested a review from refack April 3, 2019 02:06
refack
refack suggested changes Apr 3, 2019
View reviewed changes
src/node_platform.cc Outdated Show resolved Hide resolved
@gengjiawen gengjiawen force-pushed the bugprone-incorrect-roundings branch from 9ce6d45 to eea7aab Compare April 3, 2019 13:11
@nodejs-github-bot
Copy link
Collaborator

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

@gengjiawen @refack
src: apply clang-tidy rule bugprone-incorrect-roundings
d711b97 
PR-URL: nodejs#26885
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@refack refack force-pushed the bugprone-incorrect-roundings branch from eea7aab to d711b97 Compare April 3, 2019 16:20
@refack refack merged commit d711b97 into nodejs:master Apr 3, 2019
@gengjiawen gengjiawen deleted the bugprone-incorrect-roundings branch April 3, 2019 16:41
BethGriggs pushed a commit that referenced this pull request Apr 4, 2019
@gengjiawen @BethGriggs
src: apply clang-tidy rule bugprone-incorrect-roundings
49337b0 
PR-URL: #26885
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 9, 2019
@gengjiawen @BethGriggs
src: apply clang-tidy rule bugprone-incorrect-roundings
c537daf 
PR-URL: #26885
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Signed-off-by: Beth Griggs <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Apr 9, 2019
Merged
BethGriggs pushed a commit that referenced this pull request Apr 10, 2019
@gengjiawen @BethGriggs
src: apply clang-tidy rule bugprone-incorrect-roundings
438bc52 
PR-URL: #26885
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Signed-off-by: Beth Griggs <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@JungMinu JungMinu JungMinu approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@gengjiawen @nodejs-github-bot @mscdex @refack @jasnell @JungMinu