Fix float comparaison and add float comparison warning #1368
Conversation
Fix float comparaison and add float comparison warning Signed-off-by: Bensuperpc <bensuperpc@gmail.com>
| @@ -195,7 +195,7 @@ void State::PauseTiming() { | |||
| for (const auto& name_and_measurement : measurements) { | |||
| auto name = name_and_measurement.first; | |||
| auto measurement = name_and_measurement.second; | |||
| BM_CHECK_EQ(counters[name], 0.0); | |||
There was a problem hiding this comment.
What is the type of counters[name]?
There was a problem hiding this comment.
What is the type of
counters[name]?
counters[name] seems to return a float value
counters is std::map<std::string, Counter> type
Maybe this is more relevant ? Rather than the solution I used:
BM_CHECK_FLOAT_EQ(static_cast<int>(counters[name]), 0.0, std::numeric_limits<double>::epsilon());
There was a problem hiding this comment.
What happens if you instead change it to
BM_CHECK_EQ(counters[name], 0.0f);
?
There was a problem hiding this comment.
Update: change with BM_CHECK_EQ(counters[name], 0.0f);
This produces the warning (-Wfloat-equal):
[bensuperpc@bensuperpc benchmark]$ cmake -S . -B build -G Ninja -D BENCHMARK_DOWNLOAD_DEPENDENCIES=true -D BENCHMARK_ENABLE_LIBPFM=1 && ninja -C build
-- LLVM FileCheck Found: /usr/bin/FileCheck
-- git version: v1.6.0-80-ge7aa41fc-dirty normalized to 1.6.0.80
-- Version: 1.6.0.80
-- Performing Test HAVE_STD_REGEX -- success
-- Performing Test HAVE_GNU_POSIX_REGEX -- failed to compile
-- Performing Test HAVE_POSIX_REGEX -- success
-- Performing Test HAVE_STEADY_CLOCK -- success
Perf Counters support requested, but was unable to find libpfm.
-- Looking for Google Test sources
-- Looking for Google Test sources in Depot_git/repos_perso/benchmark/googletest
CMake Warning at CMakeLists.txt:37 (message):
Did not find Google Test sources! Fetching from web...
-- Configuring done
-- Generating done
-- Build files have been written to: Depot_git/repos_perso/benchmark/build/third_party/googletest
[1/7] Performing update step for 'googletest'
[2/7] No patch step for 'googletest'
[3/7] No configure step for 'googletest'
[4/7] No build step for 'googletest'
[5/7] No install step for 'googletest'
[6/7] No test step for 'googletest'
[7/7] Completed 'googletest'
-- Configuring done
-- Generating done
-- Build files have been written to: Depot_git/repos_perso/benchmark/build
ninja: Entering directory `build'
[1/41] Building CXX object src/CMakeFiles/benchmark.dir/benchmark.cc.o
Dans le fichier inclus depuis Depot_git/repos_perso/benchmark/src/perf_counters.h:24,
depuis Depot_git/repos_perso/benchmark/src/benchmark_runner.h:23,
depuis Depot_git/repos_perso/benchmark/src/benchmark.cc:18:
Depot_git/repos_perso/benchmark/src/benchmark.cc: Dans la fonction membre « void benchmark::State::PauseTiming() »:
Depot_git/repos_perso/benchmark/src/check.h:74:40: attention: comparer des nombres flottants à l'aide de « == » ou « != » n'est pas sûr [-Wfloat-equal]
74 | #define BM_CHECK_EQ(a, b) BM_CHECK((a) == (b))
Depot_git/repos_perso/benchmark/src/check.h:65:4: note: dans la définition de la macro « BM_CHECK »
65 | (b ? ::benchmark::internal::GetNullLogInstance() \
| ^
Depot_git/repos_perso/benchmark/src/benchmark.cc:198:7: note: dans l'expansion de la macro « BM_CHECK_EQ »
198 | BM_CHECK_EQ(counters[name], 0.0f);
| ^~~~~~~~~~~
[41/41] Linking CXX executable test/benchmark_gtest
There was a problem hiding this comment.
That's not what i asked, please read my previous inline comment again.
There was a problem hiding this comment.
That's not what i asked, please read my previous inline comment again.
Sorry, i misread :/
I updated my previous comment and I pushed the changes, the warning is still there
There was a problem hiding this comment.
Alright. That is a bad diagnostic then.
Let's just do BM_CHECK_EQ(std::fpclassify(counters[name]), FP_ZERO); then i guess.
There was a problem hiding this comment.
Alright. That is a bad diagnostic then. Let's just do
BM_CHECK_EQ(std::fpclassify(counters[name]), FP_ZERO);then i guess.
The warning is no longer there when I compile, this causes some problems on CI
There was a problem hiding this comment.
I'm not sure if libc++ is correct or not, but i guess we need BM_CHECK_EQ(std::fpclassify((double)counters[name]), FP_ZERO);
There was a problem hiding this comment.
I'm not sure if libc++ is correct or not, but i guess we need
BM_CHECK_EQ(std::fpclassify((double)counters[name]), FP_ZERO);
Filed llvm/llvm-project#54356
Signed-off-by: Bensuperpc <bensuperpc@gmail.com>
Signed-off-by: Bensuperpc <bensuperpc@gmail.com>
Change with BM_CHECK_EQ(std::fpclassify(counters[name]), FP_ZERO); Signed-off-by: Bensuperpc <bensuperpc@gmail.com>
|
This is a GCC bug, please report it upstream: https://godbolt.org/z/qbWb61n3Y |
Change with BM_CHECK_EQ(std::fpclassify((double)counters[name]), FP_ZERO); Signed-off-by: Bensuperpc <bensuperpc@gmail.com>
Fix float comparaison and add float comparison warning
Fix error in my CI with -Werror=float-equal flag: MathLib
Signed-off-by: Bensuperpc bensuperpc@gmail.com