From 4df610bd337c729dbb3cea28bb173dcf15f2068b Mon Sep 17 00:00:00 2001 From: Dominic Hamon Date: Wed, 6 Mar 2024 13:42:03 +0000 Subject: [PATCH 1/6] mitigate clang build warnings -Wconversion --- test/complexity_test.cc | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/test/complexity_test.cc b/test/complexity_test.cc index 0c159cd27d..0729d15aa7 100644 --- a/test/complexity_test.cc +++ b/test/complexity_test.cc @@ -71,11 +71,11 @@ void BM_Complexity_O1(benchmark::State &state) { for (auto _ : state) { // This test requires a non-zero CPU time to avoid divide-by-zero benchmark::DoNotOptimize(state.iterations()); - double tmp = state.iterations(); + double tmp = static_cast(state.iterations()); benchmark::DoNotOptimize(tmp); for (benchmark::IterationCount i = 0; i < state.iterations(); ++i) { benchmark::DoNotOptimize(state.iterations()); - tmp *= state.iterations(); + tmp *= static_cast(state.iterations()); benchmark::DoNotOptimize(tmp); } @@ -120,16 +120,16 @@ void BM_Complexity_O_N(benchmark::State &state) { for (auto _ : state) { // This test requires a non-zero CPU time to avoid divide-by-zero benchmark::DoNotOptimize(state.iterations()); - double tmp = state.iterations(); + double tmp = static_cast(state.iterations()); benchmark::DoNotOptimize(tmp); for (benchmark::IterationCount i = 0; i < state.iterations(); ++i) { benchmark::DoNotOptimize(state.iterations()); - tmp *= state.iterations(); + tmp *= static_cast(state.iterations()); benchmark::DoNotOptimize(tmp); } // 1ns per iteration per entry - state.SetIterationTime(state.range(0) * 42 * 1e-9); + state.SetIterationTime(static_cast(state.range(0)) * 42 * 1e-9); } state.SetComplexityN(state.range(0)); } @@ -178,16 +178,16 @@ static void BM_Complexity_O_N_log_N(benchmark::State &state) { for (auto _ : state) { // This test requires a non-zero CPU time to avoid divide-by-zero benchmark::DoNotOptimize(state.iterations()); - double tmp = state.iterations(); + double tmp = static_cast(state.iterations()); benchmark::DoNotOptimize(tmp); for (benchmark::IterationCount i = 0; i < state.iterations(); ++i) { benchmark::DoNotOptimize(state.iterations()); - tmp *= state.iterations(); + tmp *= static_cast(state.iterations()); benchmark::DoNotOptimize(tmp); } - state.SetIterationTime(state.range(0) * kLog2E * std::log(state.range(0)) * - 42 * 1e-9); + state.SetIterationTime(static_cast(state.range(0)) * kLog2E * + std::log(state.range(0)) * 42 * 1e-9); } state.SetComplexityN(state.range(0)); } @@ -238,15 +238,15 @@ void BM_ComplexityCaptureArgs(benchmark::State &state, int n) { for (auto _ : state) { // This test requires a non-zero CPU time to avoid divide-by-zero benchmark::DoNotOptimize(state.iterations()); - double tmp = state.iterations(); + double tmp = static_cast(state.iterations()); benchmark::DoNotOptimize(tmp); for (benchmark::IterationCount i = 0; i < state.iterations(); ++i) { benchmark::DoNotOptimize(state.iterations()); - tmp *= state.iterations(); + tmp *= static_cast(state.iterations()); benchmark::DoNotOptimize(tmp); } - state.SetIterationTime(state.range(0) * 42 * 1e-9); + state.SetIterationTime(static_cast(state.range(0)) * 42 * 1e-9); } state.SetComplexityN(n); } From 258af5bfb20d063d7134b53424b8143d743ac140 Mon Sep 17 00:00:00 2001 From: Dominic Hamon Date: Wed, 6 Mar 2024 15:10:37 +0000 Subject: [PATCH 2/6] ensure we have warnings set everywhere and fix some --- BUILD.bazel | 19 ++++++++++++++++++- CMakeLists.txt | 1 + include/benchmark/benchmark.h | 2 +- src/benchmark.cc | 3 ++- src/benchmark_register.cc | 5 ++--- src/benchmark_runner.cc | 2 +- src/cycleclock.h | 2 +- src/statistics.cc | 4 ++-- src/string_util.cc | 2 +- src/sysinfo.cc | 4 ++-- src/timers.cc | 4 ++-- test/BUILD | 1 + 12 files changed, 34 insertions(+), 15 deletions(-) diff --git a/BUILD.bazel b/BUILD.bazel index d72ae86728..15d836998c 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -1,5 +1,22 @@ licenses(["notice"]) +COPTS = [ + "-pedantic", + "-pedantic-errors", + "-std=c++11", + "-Wall", + "-Wconversion", + "-Wextra", + "-Wshadow", + # "-Wshorten-64-to-32", + "-Wfloat-equal", + "-fstrict-aliasing", + ## assert() are used a lot in tests upstream, which may be optimised out leading to + ## unused-variable warning. + "-Wno-unused-variable", + "-Werror=old-style-cast", +] + config_setting( name = "qnx", constraint_values = ["@platforms//os:qnx"], @@ -47,7 +64,7 @@ cc_library( ], copts = select({ ":windows": [], - "//conditions:default": ["-Werror=old-style-cast"], + "//conditions:default": COPTS, }), defines = [ "BENCHMARK_STATIC_DEFINE", diff --git a/CMakeLists.txt b/CMakeLists.txt index d9bcc6a493..23b519c250 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -190,6 +190,7 @@ else() add_cxx_compiler_flag(-Wshadow) add_cxx_compiler_flag(-Wfloat-equal) add_cxx_compiler_flag(-Wold-style-cast) + add_cxx_compiler_flag(-Wconversion) if(BENCHMARK_ENABLE_WERROR) add_cxx_compiler_flag(-Werror) endif() diff --git a/include/benchmark/benchmark.h b/include/benchmark/benchmark.h index 08cfe29da3..656b966827 100644 --- a/include/benchmark/benchmark.h +++ b/include/benchmark/benchmark.h @@ -1304,7 +1304,7 @@ class BENCHMARK_EXPORT Benchmark { public: const char* GetName() const; int ArgsCnt() const; - const char* GetArgName(int arg) const; + const char* GetArgName(size_t arg) const; private: friend class BenchmarkFamilies; diff --git a/src/benchmark.cc b/src/benchmark.cc index 563c443800..1f2f6cc277 100644 --- a/src/benchmark.cc +++ b/src/benchmark.cc @@ -407,7 +407,8 @@ void RunBenchmarks(const std::vector& benchmarks, benchmarks_with_threads += (benchmark.threads() > 1); runners.emplace_back(benchmark, &perfcounters, reports_for_family); int num_repeats_of_this_instance = runners.back().GetNumRepeats(); - num_repetitions_total += num_repeats_of_this_instance; + num_repetitions_total += + static_cast(num_repeats_of_this_instance); if (reports_for_family) reports_for_family->num_runs_total += num_repeats_of_this_instance; } diff --git a/src/benchmark_register.cc b/src/benchmark_register.cc index e447c9a2d3..d193e37c72 100644 --- a/src/benchmark_register.cc +++ b/src/benchmark_register.cc @@ -480,9 +480,8 @@ int Benchmark::ArgsCnt() const { return static_cast(args_.front().size()); } -const char* Benchmark::GetArgName(int arg) const { - BM_CHECK_GE(arg, 0); - BM_CHECK_LT(arg, static_cast(arg_names_.size())); +const char* Benchmark::GetArgName(size_t arg) const { + BM_CHECK_LT(arg, arg_names_.size()); return arg_names_[arg].c_str(); } diff --git a/src/benchmark_runner.cc b/src/benchmark_runner.cc index dcddb437e3..a74bdadd3e 100644 --- a/src/benchmark_runner.cc +++ b/src/benchmark_runner.cc @@ -235,7 +235,7 @@ BenchmarkRunner::BenchmarkRunner( has_explicit_iteration_count(b.iterations() != 0 || parsed_benchtime_flag.tag == BenchTimeType::ITERS), - pool(b.threads() - 1), + pool(static_cast(b.threads() - 1)), iters(has_explicit_iteration_count ? ComputeIters(b_, parsed_benchtime_flag) : 1), diff --git a/src/cycleclock.h b/src/cycleclock.h index eff563e7fa..71eaef90ac 100644 --- a/src/cycleclock.h +++ b/src/cycleclock.h @@ -82,7 +82,7 @@ inline BENCHMARK_ALWAYS_INLINE int64_t Now() { #elif defined(__x86_64__) || defined(__amd64__) uint64_t low, high; __asm__ volatile("rdtsc" : "=a"(low), "=d"(high)); - return (high << 32) | low; + return static_cast((high << 32) | low); #elif defined(__powerpc__) || defined(__ppc__) // This returns a time-base, which is not always precisely a cycle-count. #if defined(__powerpc64__) || defined(__ppc64__) diff --git a/src/statistics.cc b/src/statistics.cc index 261dcb299a..16b60261fd 100644 --- a/src/statistics.cc +++ b/src/statistics.cc @@ -97,7 +97,7 @@ std::vector ComputeStats( auto error_count = std::count_if(reports.begin(), reports.end(), [](Run const& run) { return run.skipped; }); - if (reports.size() - error_count < 2) { + if (reports.size() - static_cast(error_count) < 2) { // We don't report aggregated data if there was a single run. return results; } @@ -179,7 +179,7 @@ std::vector ComputeStats( // Similarly, if there are N repetitions with 1 iterations each, // an aggregate will be computed over N measurements, not 1. // Thus it is best to simply use the count of separate reports. - data.iterations = reports.size(); + data.iterations = static_cast(reports.size()); data.real_accumulated_time = Stat.compute_(real_accumulated_time_stat); data.cpu_accumulated_time = Stat.compute_(cpu_accumulated_time_stat); diff --git a/src/string_util.cc b/src/string_util.cc index c69e40a813..9ba63a700a 100644 --- a/src/string_util.cc +++ b/src/string_util.cc @@ -56,7 +56,7 @@ void ToExponentAndMantissa(double val, int precision, double one_k, scaled /= one_k; if (scaled <= big_threshold) { mantissa_stream << scaled; - *exponent = i + 1; + *exponent = static_cast(i + 1); *mantissa = mantissa_stream.str(); return; } diff --git a/src/sysinfo.cc b/src/sysinfo.cc index daeb98b026..58b4c45c10 100644 --- a/src/sysinfo.cc +++ b/src/sysinfo.cc @@ -120,7 +120,7 @@ struct ValueUnion { explicit ValueUnion(std::size_t buff_size) : size(sizeof(DataT) + buff_size), - buff(::new (std::malloc(size)) DataT(), &std::free) {} + buff(::new(std::malloc(size)) DataT(), &std::free) {} ValueUnion(ValueUnion&& other) = default; @@ -837,7 +837,7 @@ std::vector GetLoadAvg() { !(defined(__ANDROID__) && __ANDROID_API__ < 29) static constexpr int kMaxSamples = 3; std::vector res(kMaxSamples, 0.0); - const int nelem = getloadavg(res.data(), kMaxSamples); + const size_t nelem = static_cast(getloadavg(res.data(), kMaxSamples)); if (nelem < 1) { res.clear(); } else { diff --git a/src/timers.cc b/src/timers.cc index 667e7b2eef..d0821f3166 100644 --- a/src/timers.cc +++ b/src/timers.cc @@ -245,9 +245,9 @@ std::string LocalDateTimeString() { tz_offset_sign = '-'; } - tz_len = + tz_len = static_cast( ::snprintf(tz_offset, sizeof(tz_offset), "%c%02li:%02li", - tz_offset_sign, offset_minutes / 100, offset_minutes % 100); + tz_offset_sign, offset_minutes / 100, offset_minutes % 100)); BM_CHECK(tz_len == kTzOffsetLen); ((void)tz_len); // Prevent unused variable warning in optimized build. } else { diff --git a/test/BUILD b/test/BUILD index e43b802350..b245fa7622 100644 --- a/test/BUILD +++ b/test/BUILD @@ -21,6 +21,7 @@ TEST_COPTS = [ ## assert() are used a lot in tests upstream, which may be optimised out leading to ## unused-variable warning. "-Wno-unused-variable", + "-Werror=old-style-cast", ] # Some of the issues with DoNotOptimize only occur when optimization is enabled From 8af8a3c0d9b0e5be5510c781e3530fbf57d3a8d1 Mon Sep 17 00:00:00 2001 From: Dominic Hamon Date: Wed, 6 Mar 2024 15:12:53 +0000 Subject: [PATCH 3/6] clang format --- src/sysinfo.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sysinfo.cc b/src/sysinfo.cc index 58b4c45c10..3b53f94e49 100644 --- a/src/sysinfo.cc +++ b/src/sysinfo.cc @@ -120,7 +120,7 @@ struct ValueUnion { explicit ValueUnion(std::size_t buff_size) : size(sizeof(DataT) + buff_size), - buff(::new(std::malloc(size)) DataT(), &std::free) {} + buff(::new (std::malloc(size)) DataT(), &std::free) {} ValueUnion(ValueUnion&& other) = default; From ddf1ec9ae739d2428afa0001c75b15efc605ebbb Mon Sep 17 00:00:00 2001 From: Dominic Hamon Date: Wed, 6 Mar 2024 15:46:26 +0000 Subject: [PATCH 4/6] fix more warnings --- include/benchmark/benchmark.h | 2 +- src/benchmark_register.cc | 8 +++++--- test/benchmark_gtest.cc | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/include/benchmark/benchmark.h b/include/benchmark/benchmark.h index 656b966827..08cfe29da3 100644 --- a/include/benchmark/benchmark.h +++ b/include/benchmark/benchmark.h @@ -1304,7 +1304,7 @@ class BENCHMARK_EXPORT Benchmark { public: const char* GetName() const; int ArgsCnt() const; - const char* GetArgName(size_t arg) const; + const char* GetArgName(int arg) const; private: friend class BenchmarkFamilies; diff --git a/src/benchmark_register.cc b/src/benchmark_register.cc index d193e37c72..8ade048225 100644 --- a/src/benchmark_register.cc +++ b/src/benchmark_register.cc @@ -480,9 +480,11 @@ int Benchmark::ArgsCnt() const { return static_cast(args_.front().size()); } -const char* Benchmark::GetArgName(size_t arg) const { - BM_CHECK_LT(arg, arg_names_.size()); - return arg_names_[arg].c_str(); +const char* Benchmark::GetArgName(int arg) const { + BM_CHECK_GE(arg, 0); + size_t uarg = static_cast(arg); + BM_CHECK_LT(uarg, arg_names_.size()); + return arg_names_[uarg].c_str(); } TimeUnit Benchmark::GetTimeUnit() const { diff --git a/test/benchmark_gtest.cc b/test/benchmark_gtest.cc index 2c9e555d92..0aa2552c1e 100644 --- a/test/benchmark_gtest.cc +++ b/test/benchmark_gtest.cc @@ -38,7 +38,7 @@ TEST(AddRangeTest, Advanced64) { TEST(AddRangeTest, FullRange8) { std::vector dst; - AddRange(&dst, int8_t{1}, std::numeric_limits::max(), int8_t{8}); + AddRange(&dst, int8_t{1}, std::numeric_limits::max(), 8); EXPECT_THAT( dst, testing::ElementsAre(int8_t{1}, int8_t{8}, int8_t{64}, int8_t{127})); } From 074489b4452426559e9e89f7dd50993c54fa8b2c Mon Sep 17 00:00:00 2001 From: Dominic Hamon Date: Wed, 6 Mar 2024 16:07:03 +0000 Subject: [PATCH 5/6] more fixes --- src/benchmark_register.h | 4 ++-- src/cycleclock.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/benchmark_register.h b/src/benchmark_register.h index 53367c707c..be50265f72 100644 --- a/src/benchmark_register.h +++ b/src/benchmark_register.h @@ -24,7 +24,7 @@ typename std::vector::iterator AddPowers(std::vector* dst, T lo, T hi, static const T kmax = std::numeric_limits::max(); // Space out the values in multiples of "mult" - for (T i = static_cast(1); i <= hi; i *= static_cast(mult)) { + for (T i = static_cast(1); i <= hi; i = static_cast(i * mult)) { if (i >= lo) { dst->push_back(i); } @@ -52,7 +52,7 @@ void AddNegatedPowers(std::vector* dst, T lo, T hi, int mult) { const auto it = AddPowers(dst, hi_complement, lo_complement, mult); - std::for_each(it, dst->end(), [](T& t) { t *= -1; }); + std::for_each(it, dst->end(), [](T& t) { t = static_cast(t * -1); }); std::reverse(it, dst->end()); } diff --git a/src/cycleclock.h b/src/cycleclock.h index 71eaef90ac..91abcf9dba 100644 --- a/src/cycleclock.h +++ b/src/cycleclock.h @@ -70,7 +70,7 @@ inline BENCHMARK_ALWAYS_INLINE int64_t Now() { // frequency scaling). Also note that when the Mac sleeps, this // counter pauses; it does not continue counting, nor does it // reset to zero. - return mach_absolute_time(); + return static_cast(mach_absolute_time()); #elif defined(BENCHMARK_OS_EMSCRIPTEN) // this goes above x86-specific code because old versions of Emscripten // define __x86_64__, although they have nothing to do with it. From 8efc18efe134ba995f2ffb9bdba14a2a06d1018a Mon Sep 17 00:00:00 2001 From: Dominic Hamon Date: Wed, 6 Mar 2024 16:15:46 +0000 Subject: [PATCH 6/6] windows warnings --- src/sysinfo.cc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/sysinfo.cc b/src/sysinfo.cc index 3b53f94e49..57a23e7bc0 100644 --- a/src/sysinfo.cc +++ b/src/sysinfo.cc @@ -350,7 +350,7 @@ std::vector GetCacheSizesWindows() { CPUInfo::CacheInfo C; C.num_sharing = static_cast(b.count()); C.level = cache.Level; - C.size = cache.Size; + C.size = static_cast(cache.Size); C.type = "Unknown"; switch (cache.Type) { case CacheUnified: @@ -485,9 +485,8 @@ int GetNumCPUsImpl() { // positives. std::memset(&sysinfo, 0, sizeof(SYSTEM_INFO)); GetSystemInfo(&sysinfo); - return sysinfo.dwNumberOfProcessors; // number of logical - // processors in the current - // group + // number of logical processors in the current group + return static_cast(sysinfo.dwNumberOfProcessors); #elif defined(BENCHMARK_OS_SOLARIS) // Returns -1 in case of a failure. long num_cpu = sysconf(_SC_NPROCESSORS_ONLN);