google / benchmark

A microbenchmark support library
Apache License 2.0
8.94k stars 1.62k forks source link

[BUG] DoNotOptimize `const &` overload chosen for non-const lvalue references #1619

Closed eseiler closed 1 year ago

eseiler commented 1 year ago

Describe the bug After df9a99d998d9f038a53a970a77ce31c8c2a882ce, const & overloads are chosen where previously the & overload was chosen. This leads to deprecation warnings where IMO there should be none. const & takes precedence over the new && overload.

Godbolt: https://godbolt.org/z/5s6sKT7dc

Click to show code from godbolt example ```cpp #include #include #include static void test(benchmark::State & state) { std::vector vec(100); for (auto _ : state) { std::fill(vec.begin(), vec.end(), 1); benchmark::DoNotOptimize(vec); } } BENCHMARK(test); BENCHMARK_MAIN(); ``` ``` : In function 'void test(benchmark::State&)': :13:33: error: 'typename std::enable_if<((! std::is_trivially_copyable<_Tp>::value) || (sizeof (Tp) > sizeof (Tp*)))>::type benchmark::DoNotOptimize(const Tp&) [with Tp = std::vector; typename std::enable_if<((! std::is_trivially_copyable<_Tp>::value) || (sizeof (Tp) > sizeof (Tp*)))>::type = void]' is deprecated: The const-ref version of this method can permit undesired compiler optimizations in benchmarks [-Werror=deprecated-declarations] 13 | benchmark::DoNotOptimize(vec); | ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~ In file included from :1: /opt/compiler-explorer/libs/google-benchmark/trunk/include/benchmark/benchmark.h:502:5: note: declared here 502 | DoNotOptimize(Tp const& value) { | ^~~~~~~~~~~~~ cc1plus: all warnings being treated as errors ```

System Which OS, compiler, and compiler version are you using:

To reproduce See bug description.

Expected behavior IMO, it should still work. There probably needs to be both & and && overloads. It does work if I add additional & overloads.

eseiler commented 1 year ago

If this is confirmed to be a bug, I could offer to provide a PR with the code changes and test cases.

LebedevRI commented 1 year ago

I'm pretty sure #1608 should have only affected rvalues, so this does seem like a bug.

LebedevRI commented 1 year ago

~~Hm, i'm confused. DoNotOptimize const & overload change happened much earlier than df9a99d998d9f038a53a970a77ce31c8c2a882ce, it was already present in v1.8.0, yet i concur that i'm also only now started getting those warnings, in v1.8.1: https://github.com/darktable-org/rawspeed/actions/runs/5454117056/jobs/9923812487?pr=494~~

@dmah42 so i guess that means the original #1493 didn't prohibit all the cases?

LebedevRI commented 1 year ago

Ok, no, let me backtrack, those are false-positives, this is a bug. @eseiler patches most welcomed! Looks like we'll be getting .2 soon? (: