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/features/src/matching.cc

Summary:
# Overview

LLVM has detected a bad _implicit constant integer to float conversion_ in mapillary/opensfm/opensfm/src/features/src/matching.cc. 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: YanNoun

Differential Revision: D54586513

fbshipit-source-id: 87fba3097e3822eb68660e0f2ab947eaf6450698
  • Loading branch information
r-barnes authored and facebook-github-bot committed Mar 6, 2024
1 parent 7ad7339 commit 938a3a1
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions opensfm/src/features/src/matching.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ void MatchUsingWords(const cv::Mat &f1, const cv::Mat &w1, const cv::Mat &f2,
}

std::vector<int> best_match(f1.rows, -1), second_best_match(f1.rows, -1);
std::vector<float> best_distance(f1.rows, 99999999),
second_best_distance(f1.rows, 99999999);
std::vector<float> best_distance(f1.rows, std::numeric_limits<float>::infinity());
std::vector<float> second_best_distance(f1.rows, std::numeric_limits<float>::infinity());
*matches = cv::Mat(0, 2, CV_32S);
cv::Mat tmp_match(1, 2, CV_32S);
for (unsigned int i = 0; i < w1.rows; ++i) {
Expand Down

0 comments on commit 938a3a1

Please sign in to comment.