Skip to content

Commit

Permalink
Fix -Wimplicit-const-int-float-conversion violation in mapillary/op…
Browse files Browse the repository at this point in the history
…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
r-barnes authored and facebook-github-bot committed Mar 7, 2024
1 parent 424ecc2 commit afb0c15
Showing 1 changed file with 2 additions and 1 deletion.
3 changes: 2 additions & 1 deletion opensfm/src/geometry/relative_pose.h
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ struct RelativePoseCost {
std::srand(42);
const int count = end_ - begin_;
for (int i = 0; i < MAX_ERRORS; ++i) {
const int index = (float(std::rand()) / RAND_MAX) * count;
// Note that float(RAND_MAX) cannot be exactly represented as a float. We ignore the small inaccuracy here; this is already a bad way to get random numbers.
const int index = (float(std::rand()) / static_cast<float>(RAND_MAX)) * count;
picked_errors_.push_back(begin_ + index);
}
}
Expand Down

0 comments on commit afb0c15

Please sign in to comment.