Better use of std::variant #2714
Conversation
Signed-off-by: Maksim Derbasov <[email protected]>
| @@ -221,7 +221,7 @@ | |||
| </link> | |||
|
|
|||
| <link name='front_right_wheel'> | |||
| <pose>0.554282 -0.625029 -0.025 -1.5707 0 0</pose> | |||
| <pose>0.554283 -0.625029 -0.025 -1.5707 0 0</pose> | |||
There was a problem hiding this comment.
in other 3 places in file it's 0.554283, but here it's 0.554282, aligned to simplify find-replace for users
| } catch (std::bad_variant_access &) | ||
| } | ||
| catch (std::bad_variant_access &) |
There was a problem hiding this comment.
fixed codestyle and conveniently it's one of the cases where I suppose try-catch block is redundant
Whole list:
TransformControl.cc:479
TransformControl.cc:558
TransformControl.cc:592
TransformControl.cc:681
TransformControl.cc:694
TransformControl.cc:716
AFAIU, SetUserData is belong to gz-rendering/base/BaseNode.hh, which just assigning to std::variant, which is inside of std::map; and operator= can throw only if type which assigning throwing something
I can be wrong
There was a problem hiding this comment.
I think it may not be necessary as well. There are other places in the code base that don't wrap SetUserData with try/catch
There was a problem hiding this comment.
Ok, I'll clean-up it
Quite funny, compiler can optimize a try-catch block if it can see all side effects https://godbolt.org/z/7co957W1h
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## gz-sim9 #2714 +/- ##
==========================================
Coverage ? 68.80%
==========================================
Files ? 341
Lines ? 33076
Branches ? 0
==========================================
Hits ? 22759
Misses ? 10317
Partials ? 0 ☔ View full report in Codecov by Sentry. |
| @@ -47,12 +47,12 @@ | |||
| using namespace gz; | |||
| using namespace gz::sim; | |||
|
|
|||
| /// Sensor prefix to be used. All envionment_sensors are to be prefixed by | |||
| /// Sensor prefix to be used. All enviornment_sensors are to be prefixed by | |||
There was a problem hiding this comment.
| /// Sensor prefix to be used. All enviornment_sensors are to be prefixed by | |
| /// Sensor prefix to be used. All environment_sensors are to be prefixed by |
| } catch (std::bad_variant_access &) | ||
| } | ||
| catch (std::bad_variant_access &) |
There was a problem hiding this comment.
I think it may not be necessary as well. There are other places in the code base that don't wrap SetUserData with try/catch
Signed-off-by: Maksim Derbasov <[email protected]>
|
Maybe we can wrap-up it? I have a fix for a flakiness in test/integration/battery_plugin.cc and fix for lost threading sync for src/systems/battery_plugin/LinearBatteryPlugin.cc (yes, I touched this file before due to I spotted instability there but it took time to figure out what's the problem) |
🦟 Bug fix
Fixes #
Summary
std::holds_alternativeas a more simple way to improve this part of codeconstaround my changes,.empty()instead of== ""or use.front()instead of iterator to*begin(). Yes, compiler can optimize it in release mode, but in debug it will be exactly)Sorry for combining it in a one PR, splitting it apart will be too tedious I suppose (and for me, and fore maintainers)
Checklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-bymessages.