Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Fix integer division in transform.cpp #1406

Merged
merged 2 commits into from
May 4, 2015
Merged

Fix integer division in transform.cpp #1406

merged 2 commits into from
May 4, 2015

Conversation

springmeyer
Copy link
Contributor

Ran mbgl-native through coverity static analysis tonight. Coverity uncovered that our transform code has been doing integer division. This may be leading to loss of precision / truncated values - a classic C/C++ bug. If anyone is not familiar with this behavior have a look at: http://coliru.stacked-crooked.com/a/62dd1db484efeac8

The report looked like:

screen shot 2015-05-03 at 11 30 59 pm

Fuller report at https://scan4.coverity.com/reports.htm#v13662/p11099 (you should be able to login via github account).

Overall: I hit two coverity bugs that prevented most files from being able to be processed that I've emailed coverity support about. It looks like coverity does not support atomic and there are some libc++ coverity issues. I'll post more bug reports when I can process the entire libmgbl-core.

springmeyer pushed a commit that referenced this pull request May 4, 2015
Fix integer division in transform.cpp
@springmeyer springmeyer merged commit c6be708 into master May 4, 2015
@springmeyer springmeyer deleted the coverity-fixes branch May 4, 2015 06:49
@incanus
Copy link
Contributor

incanus commented May 4, 2015

Nice @springmeyer, this is enlightening.

@springmeyer
Copy link
Contributor Author

btw, you would think that -Wsign-conversion or -Wconversion would warn on this, but it does not in all cases (http://stackoverflow.com/questions/6076878/locating-numerical-errors-due-to-integer-division). So, we need to be smart about always using explicitly using static_cast when changing types. This will help prevent problems at the time you write the code and especially when others copy and paste to refactor.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants