search

gazebosim / gz-sim

Open source robotics simulator. The latest version of Gazebo.
https://gazebosim.org
Apache License 2.0
619 stars 251 forks source link

Use DynamicCastMessage instead of dynamic_cast to downcast protos in … #2446

Closed shameekganguly closed 2 weeks ago

shameekganguly commented 2 weeks ago

🦟 Bug fix

Summary

The UserCommands system uses built-in dynamic_cast to downcast proto messages. However the currently recommended cast method for protos is DynamicCastMessage: https://github.com/protocolbuffers/protobuf/blob/40ffd46cec1291e1320e46a134c47dd23a74ff43/src/google/protobuf/message_lite.h#L1018

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

azeey commented 2 weeks ago

The version of protobuf we use in Ubuntu Jammy and Noble doesn't have this function.

scpeters commented 2 weeks ago

The version of protobuf we use in Ubuntu Jammy and Noble doesn't have this function.

This was pushed to protobuf's main branch 3 weeks ago in https://github.com/protocolbuffers/protobuf/commit/18da465815e609732b902d8cbc79d2d12e90686d. It looks like that renamed the API from DynamicCastToGenerated to DynamicCastMessage, but it has not yet had a stable release. It was not included in v27.1, so I think we could assume it will be in v28.0. We could add an #ifdef based on the protobuf version to select between these APIs instead of dynamic_cast.

shameekganguly commented 2 weeks ago

Yep, seems that way. Its ok, I'll add the methods to copybara when sync'ing to the internal repo for now and drop the PR. We can revisit it in a year. Adding an ifdef everywhere will really hamper code readability.

scpeters commented 2 weeks ago

Adding an ifdef everywhere will really hamper code readability.

we could potentially define a type alias with using inside an ifdef in one place instead of using ifdefs all over, so it could still be readable

scpeters commented 2 weeks ago

Adding an ifdef everywhere will really hamper code readability.

we could potentially define a type alias with using inside an ifdef in one place instead of using ifdefs all over, so it could still be readable

actually DynamicCastMessage isn't a class, so I don't think we could do a template type alias. I think we'd probably have to use a macro, which isn't ideal