Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Fix
-Wimplicit-const-int-float-conversion
violation in mapillary/op…
…ensfm/opensfm/src/geometry/relative_pose.h Summary: # Overview LLVM has detected a bad _implicit constant integer to float conversion_ in mapillary/opensfm/opensfm/src/geometry/relative_pose.h. Almost all such cases lead to very subtle bugs, so we are making them a compiler error by default. # Reproducer The fix can be checked using: ``` buck2 build --config cxx.extra_cxxflags=-Wimplicit-const-int-float-conversion <TARGET> ``` # Description of the problem What value do you think this produces? ``` static_cast<uint64_t>(static_cast<double>(std::numeric_limits<uint64_t>::max())) ``` You'd hope it would produce UINT64_MAX (18446744073709551615), but it doesn't. In debug mode it produces 9223372036854775808 (half of the above). In opt mode it produces "random" numbers such as 140726009322632 (which is 130,000x smaller than UINT64_MAX). Comparisons between floats and maximum integer values are also scary: ``` constexpr double x = std::numeric_limits<uint64_t>::max(); constexpr auto kMax64 = std::numeric_limits<uint64_t>::max(); std::cout << (x > kMax64 ? kMax64 : static_cast<uint64_t>(x)) << std::endl; ``` produces garbage: 140731185888920 The reason this happens is because floating-point types can only represent integers sparsely in the regions of the maximum integers (generated with P1188399781): ``` delta = 128 2147483264 f 2147483392 f 2147483520 f 2147483647 <- Largest integer 2147483648 f 2147483904 f 2147484160 f delta = 256 4294966528 f 4294966784 f 4294967040 f 4294967295 <- Largest integer 4294967296 f 4294967808 f 4294968320 f delta = 1024 9223372036854772736 d 9223372036854773760 d 9223372036854774784 d 9223372036854775807 <- Largest integer 9223372036854775808 d 9223372036854777856 d 9223372036854779904 d delta = 2048 18446744073709545472 d 18446744073709547520 d 18446744073709549568 d 18446744073709551615 <- Largest integer 18446744073709551616 d 18446744073709555712 d 18446744073709559808 d ``` # Fixing the Problem See example fixes in D54119898, D54119901, D54119899. ## Careful Clamping The fix will be situation dependent, `folly::constexpr_clamp_cast` is _very_ useful: ``` // Include the target `//folly:constexpr_math` #include <folly/ConstexprMath.h> #include <iostream> #include <limits> int main() { constexpr auto kMax32 = std::numeric_limits<int32_t>::max(); std::cout << folly::constexpr_clamp_cast<int32_t>(static_cast<float>(kMax32)) << std::endl; return 0; } ``` ## Use infinity In some cases we want want something like ``` double smallest_value = BIG_NUMBER; for (const auto &x : values) { smallest_value = std::min(smallest_value, x); } ``` If `BIG_NUMBER` is an integer this code could be incorrect! Instead we use ``` double smallest_value = std::numeric_limits<double>::infinity(); ``` ## Make the conversion explicit For situations in which we are dividing by a very large integer, we can use `static_cast<FLOAT_TYPE>(LARGE_INT)` to make the conversion from int to float explicit. The conversion is _not_ exact, but our hope is that the precision we've lost is small enough that it doesn't matter for the problem being solved. Reviewed By: dmm-fb Differential Revision: D54586517 fbshipit-source-id: b03d0e05edc5dccff17811e8419548cfb72759b0
- Loading branch information