Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix pass rvalue to DoNotOptimize #1608

Merged
merged 5 commits into from
Jun 19, 2023
Merged

Fix pass rvalue to DoNotOptimize #1608

merged 5 commits into from
Jun 19, 2023

Conversation

bgaifullin
Copy link
Contributor

@bgaifullin bgaifullin commented Jun 12, 2023 •
edited
Loading

Fixes #1584

dmah42
dmah42 reviewed Jun 12, 2023
View reviewed changes
include/benchmark/benchmark.h Outdated
@@ -465,7 +465,7 @@ inline BENCHMARK_ALWAYS_INLINE void DoNotOptimize(Tp const& value) {
}

template <class Tp>
inline BENCHMARK_ALWAYS_INLINE void DoNotOptimize(Tp& value) {
inline BENCHMARK_ALWAYS_INLINE void DoNotOptimize(Tp&& value) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i believe this should be inside the defined(BENCHMARK_HAS_CXX11) section below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@bgaifullin
Copy link
Contributor Author

windows build failed with error

Visual C++ build tools seems to be installed at C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC 
But Bazel can't find the following tools: 
    VCVARSALL.BAT, cl.exe, link.exe, lib.exe, ml64.exe 
for x64 target architecture

seems like it does not related to current PR

@dmah42
Copy link
Member

dmah42 commented Jun 13, 2023

windows build failed with error

Visual C++ build tools seems to be installed at C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC 
But Bazel can't find the following tools: 
    VCVARSALL.BAT, cl.exe, link.exe, lib.exe, ml64.exe 
for x64 target architecture

seems like it does not related to current PR

that looks suspiciously like github changed something in the windows image.

@dmah42
Copy link
Member

dmah42 commented Jun 13, 2023

windows build failed with error

Visual C++ build tools seems to be installed at C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC 
But Bazel can't find the following tools: 
    VCVARSALL.BAT, cl.exe, link.exe, lib.exe, ml64.exe 
for x64 target architecture

seems like it does not related to current PR

that looks suspiciously like github changed something in the windows image.

actions/runner-images#7662 may be related

@bgaifullin
Copy link
Contributor Author

@dmah42 How to restart checks (seems like issue for windows has been resolved) ?

@dmah42
Copy link
Member

dmah42 commented Jun 14, 2023

@dmah42 How to restart checks (seems like issue for windows has been resolved) ?

i've restarted them.

@dmah42
Copy link
Member

dmah42 commented Jun 14, 2023

still failing, but i'm comfortable that a) it's an issue we can't do much about and b) unlikely to be an issue based on your PR.

dmah42
dmah42 reviewed Jun 14, 2023
View reviewed changes
include/benchmark/benchmark.h
@@ -465,7 +465,13 @@ inline BENCHMARK_ALWAYS_INLINE void DoNotOptimize(Tp const& value) {
}

template <class Tp>
inline BENCHMARK_ALWAYS_INLINE void DoNotOptimize(Tp& value) {
inline BENCHMARK_ALWAYS_INLINE void DoNotOptimize(
#ifdef BENCHMARK_HAS_CXX11
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have a BENCHMARK_HAS_CXX11 block below. can we not simplify this somehow so this is folded into the version at line 510?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think about this, but in case of reordering blocks, the behaviour will be changed.

on other hand, if the BENCHMARK_HAS_CXX11 related code is moved to separate block - it causes code duplication (

dmah42
dmah42 previously approved these changes Jun 14, 2023
View reviewed changes
@LebedevRI
Copy link
Collaborator

@bgaifullin thank you!
While we are at it, could you please add a test so this does not get re-broken, please?

@bgaifullin
Copy link
Contributor Author

The windows build still failing with error about missing compiler binary. ((

@LebedevRI
Copy link
Collaborator

@dmah42 can the failing CI runs be un-required?

@dmah42 dmah42 merged commit df9a99d into google:main Jun 19, 2023
@dmah42
Copy link
Member

dmah42 commented Jun 19, 2023

thank you!

This was referenced Jul 4, 2023
Closed
Merged
@philnik777 philnik777 mentioned this pull request Nov 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LebedevRI LebedevRI LebedevRI approved these changes

@dmah42 dmah42 dmah42 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] DoNotOptimize(T &&) should not be deprecated
3 participants
@bgaifullin @dmah42 @LebedevRI