Code style update #107
Code style update #107
Conversation
khancyr
force-pushed
the
update
branch
2 times, most recently
from
46e564d
to
7cd6bfa
Compare
| @@ -97,7 +96,7 @@ class GZ_SIM_VISIBLE ArduPilotPlugin: | |||
| public: ArduPilotPlugin(); | |||
|
|
|||
| /// \brief Destructor. | |||
| public: ~ArduPilotPlugin(); | |||
| public: ~ArduPilotPlugin() final = default; | |||
There was a problem hiding this comment.
Probably not a problem here, but there can be some nasty side effects in shared libraries when defining virtual methods inline in headers. What happens is that each DLL using the header creates a copy of the destructor, so it is undefined which particular one gets called. Worse, a DLL unloading can destroy objects that are still supposed to be in scope. Biggest issue is with templates and base classes, but I now avoid this after being burned. Was a problem in the core Gazebo libraries for some time - now corrected.
| this->rotorVelocitySlowdownSim = kDefaultRotorVelocitySlowdownSim; | ||
| this->frequencyCutoff = kDefaultFrequencyCutoff; | ||
| this->samplingRate = kDefaultSamplingRate; |
There was a problem hiding this comment.
We can remove these completely. I think they are residual from aspects of the RotorS model the original plugin was adapted from. Not currently used in any code.
| @@ -285,20 +287,20 @@ class gz::sim::systems::ArduPilotPluginPrivate | |||
| { | |||
| // Extract data | |||
| double range_max = _msg.range_max(); | |||
| auto&& ranges = _msg.ranges(); | |||
| auto&& intensities = _msg.intensities(); | |||
| auto&& m_ranges = _msg.ranges(); | |||
There was a problem hiding this comment.
Gazebo has a convention that all member variables are camelCase with no m_ or _ prefix. They are then supposed to be accessed using this->memberVariable in code. Like it or loath it that's the Gazebo style (not a fan, but it's a carry over from the original version). We should stick to that to aid comparison with the built-in gz-sim plugins.
| void gz::sim::systems::ArduPilotPlugin::LoadGpsSensors( | ||
| sdf::ElementPtr /*_sdf*/, | ||
| const sdf::ElementPtr &/*_sdf*/, | ||
| gz::sim::EntityComponentManager &/*_ecm*/) | ||
| { | ||
| /* |
There was a problem hiding this comment.
I don't think we'll ever implement GPS sensors in the plugin so perhaps we could remove this and other commented code? I have experimented with using AP_DDS to add extra sensors - there is a lot more flexibility there and it doesn't involve adding further complexity to the SITL_JSON interface or opening additional ports. Different message types can be provided at custom rates, rather than require everything is sampled at the physics loop rate.
… 'std::vector::size_type' (aka 'unsigned long') [-Werror,-Wsign-conversion]
add some compiler options
Fix cast (implicit, c-style)
Comment or remove unused things
Zero init variable on need
Fix shadowing
fix fallthrough
correct clang warnings
use lambda instead of bind for better performance and safety
use range based loop when possible
fix some clang-tidy warning