search

osrf / osrf_testing_tools_cpp

Common testing tools for C++ which are used for testing in various OSRF projects.
Apache License 2.0
33 stars 29 forks source link

Make sure there is an actual side-effect. #11

Closed clalancette closed 6 years ago

clalancette commented 6 years ago

To truly keep away the optimizer, we make sure that my_first_function has an externally-visible side-effect.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

clalancette commented 6 years ago

CI (normal):

CI OSX Release (where the problem is occurring):

wjwwood commented 6 years ago

Should we merge this as-is or use volatile?

clalancette commented 6 years ago

Should we merge this as-is or use volatile?

In my testing, putting a volatile on the stack variable some_memory didn't make any difference. That said, adding it to side_effect probably won't hurt anything. I'll try it out and then post an update here.

clalancette commented 6 years ago

I added volatile in cbd40227aabb1a75a427a64906dfece7a03e0adf, and it seems to work:

https://ci.ros2.org/job/ci_osx/4058

It's slightly ugly in that I have to cast the volatile void * to void * for the memcpy, but it's not a huge deal. Like I said, volatile didn't make a different in my testing anyway, so I'm fine going either way; just let me know.

wjwwood commented 6 years ago

I'm fine with or without as well, @sloretz any opinion?

sloretz commented 6 years ago

I approved without volatile; I'm just as happy to approve with it.

wjwwood commented 6 years ago

Sounds like leave it as-is (with volatile in). Does it need new CI or are we good to go?

clalancette commented 6 years ago

Sounds like leave it as-is (with volatile in). Does it need new CI or are we good to go?

I'll do a quick CI; it only takes like 10 minutes. Assuming that is clean, I'll go ahead and merge.

clalancette commented 6 years ago
wjwwood commented 6 years ago

Thanks @clalancette and @sloretz!