Fix passing non-const lvalue refs to DoNotOptimize

[eseiler]

Resolves #1619
I re-added the & overloads that were changed to && in df9a99d. New tests do not seem necessary because the existing tests already emitted the deprecation warnings.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[LebedevRI]

Thank you!
This certainly needs tests, since we are clearly missing them.

[LebedevRI]

This certainly needs tests, since we are clearly missing them.

What i mean is that we should have noticed those new deprecation warnings.
(Is CI building without -Werror?) So -Werror should be always enabled for those tests.

[eseiler]

I'm not familiar with Bazel, and don't know where and how compiler flags are set. For CMake, the deprecations warnings are disabled:

benchmark/CMakeLists.txt

Line 192 in 408f9e0

add_cxx_compiler_flag(-Wno-deprecated-declarations)

Edit: Apparently also when BENCHMARK_ENABLE_WERROR is enabled:

benchmark/CMakeLists.txt

Lines 200 to 202 in 408f9e0

	if(BENCHMARK_ENABLE_WERROR)
	add_cxx_compiler_flag(-Wno-deprecated)
	endif()

But -Wno-deprecated does not affect the deprecated-declaration warning

[LebedevRI]

I'm not familiar with Bazel, and don't know where and how compiler flags are set.

Yep, no idea whatsoever either.

For CMake, the deprecations warnings are disabled:

Well, my point is, we should have some test, which is always compiled with -Wdeprecated -Werror,
that would have caught this bug.

[LebedevRI]

TBN, not sure we can globally enable -Wdeprecated -Werror,
because that should also break tests for methods that are deprecated...

[eseiler]

Well, my point is, we should have some test, which is always compiled with -Wdeprecated -Werror, that would have caught this bug.

I see. We would need some way to explicitly build the library with those flags. Setting compile flags for a test does not affect the library (where the error would be emitted from).

For now, I just tried not disabling those warnings (removing -Wno-deprecated-declarations). Works locally - on main an error is emitted, on this PR not. The flags were apparently disabled during some refactoring https://github.com/google/benchmark/blame/main/CMakeLists.txt#L192.
Maybe they are not necessary anymore.

[LebedevRI]

Well, my point is, we should have some test, which is always compiled with -Wdeprecated -Werror, that would have caught this bug.

I see. We would need some way to explicitly build the library with those flags. Setting compile flags for a test does not affect the library (where the error would be emitted from).

That statement makes no sense, isn't the exact opposite is true?

[eseiler]

That statement makes no sense, isn't the exact opposite is true?

Yes, it's nonsense :)

If found that target_compile_options (donotoptimize_test PRIVATE "-Wdeprecated-declarations") works.

However,

benchmark/cmake/GoogleTest.cmake

Line 36 in 408f9e0

add_compile_options(-w)

Causes tests to be compiled with -w, so nothing will be emitted.

I changed it locally to

  target_compile_options(gtest PRIVATE "-w")
  target_compile_options(gtest_main PRIVATE "-w")
  target_compile_options(gmock PRIVATE "-w")
  target_compile_options(gmock_main PRIVATE "-w")

which will cause the donotoptimize_test to emit deprecation warnings.

[LebedevRI]

However,

benchmark/cmake/GoogleTest.cmake

Line 36 in 408f9e0

add_compile_options(-w)

Causes tests to be compiled with -w, so nothing will be emitted.

Hm, are you sure? That is only for building the googletest library itself,
it should not propagate to the tests that link to that library.

[eseiler]

Hm, are you sure? That is only for building the googletest library itself, it should not propagate to the tests that link to that library.

My interpretation is:

On main, CMakeLists.txt does this:

if (BENCHMARK_ENABLE_TESTING)
  enable_testing()
  if (BENCHMARK_ENABLE_GTEST_TESTS AND
      NOT (TARGET gtest AND TARGET gtest_main AND
           TARGET gmock AND TARGET gmock_main))
    if (BENCHMARK_USE_BUNDLED_GTEST)
      include(GoogleTest)
    else()
      # ...
    endif()
  endif()
  add_subdirectory(test)
endif()

include(GoogleTest) should refer to GoogleTest.cmake. It will set -w which then is in effect for everything that follows. So, -w is set for add_subdirectory(test).
add_compile_options(-w) applies to the directory (and subdirectories), which is in this case the root directory, because the include is called from the root CMakeLists.txt.

Instead of changing the add_compile_options(-w), I can also just move the add_subdirectory(test) up such that the test directory is added before the -w flag is set.

[LebedevRI]

Huh, okay, that does appear broken indeed :|
Let's try to go with target_compile_options()?

[LebedevRI]

(if this doesn't get merged in next ~24 hours we should revert #1608 / #1584.)

[dmah42]

(if this doesn't get merged in next ~24 hours we should revert #1608 / #1584.)

i don't see why we can't merge this as soon as the bots are happy.

[dmah42]

I'm not familiar with Bazel, and don't know where and how compiler flags are set.

Yep, no idea whatsoever either.

https://github.com/google/benchmark/blob/main/test/BUILD#L10 for the options that are passed to the tests. not necessary to update here unless it's trivial.

[LebedevRI]

Nice!

Looks like the builds aren't massively failing, that's a very good sign.
Please fix clang-format nit, rest is known-broken.

[eseiler]

Formatting should be fixed

[LebedevRI]

@eseiler so i'm looking at the main branch, and if i do the CMake changes locally,
no test fails with those false-positive deprecation warnings. So we are missing a test for the bug this is fixing...

[eseiler]

Mhm, when I remove the benchmark.h changes on this branch, there are multiple errors in the donotoptimize test on my machine.

I guess I'll have to double check

[LebedevRI]

I just tried that, and i don't see any (clang16 + release build mode)

[eseiler]

Apparently, clang prefers the && overload anyway (might have to do something with the GCC overloads using a workaround).
I get the error with GCC (12), but not with clang.

So, for clang the & overload could be removed. But the same overload is also used for Intel compiler; and I don't know whether the Intel compiler needs the & overload.

[LebedevRI]

LG.
@dmah42 ?

[LebedevRI]

@eseiler thank you!

I've verified that this addresses false-positives i was seeing,
so since 1.8.1 was a dud, so after this is merged,
it might be a good idea to expedite .2.

[dmah42]

thanks

[dmah42]

(i'll release 1.8.2 tomorrow my time.. in about 12 hours)

[LebedevRI]

@eseiler @dmah42 thank you!