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

Add nodiscard and example usage in Actor #1360

Closed
wants to merge 2 commits into from
Closed

Add nodiscard and example usage in Actor #1360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d03cb17
Add nodiscard and example usage in Actor
Ryanf55 Jan 30, 2024
bc0dcec
Continue nodiscard
Ryanf55 Apr 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ if(COMMAND CMAKE_POLICY)
endif(COMMAND CMAKE_POLICY)

project (sdformat15 VERSION 15.0.0)
# set(CMAKE_CXX_CLANG_TIDY clang-tidy -checks=-*,modernize-use-nodiscard)


# The protocol version has nothing to do with the package version.
# It represents the current version of SDFormat implemented by the software
Expand Down
76 changes: 38 additions & 38 deletions include/sdf/Actor.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,39 +52,39 @@ namespace sdf

/// \brief Get the name of the animation.
/// \return Name of the animation.
public: const std::string &Name() const;
public: SDFORMAT_WARN_UNUSED const std::string &Name() const;

/// \brief Set the name of the animation.
/// \param[in] _name Name of the animation.
public: void SetName(const std::string &_name);

/// \brief Get the animation filename.
/// \return Filename of the animation.
public: const std::string &Filename() const;
public: SDFORMAT_WARN_UNUSED const std::string &Filename() const;

/// \brief Set the filename of the animation.
/// \param[in] _filename Path to animation file.
public: void SetFilename(const std::string &_filename);

/// \brief The path to the file where this element was loaded from.
/// \return Full path to the file on disk.
public: const std::string &FilePath() const;
public: SDFORMAT_WARN_UNUSED const std::string &FilePath() const;

/// \brief Set the path to the file where this element was loaded from.
/// \paramp[in] _filePath Full path to the file on disk.
public: void SetFilePath(const std::string &_filePath);

/// \brief Get the scale for the animation skeleton.
/// \return Scale of the animation skeleton.
public: double Scale() const;
public: SDFORMAT_WARN_UNUSED double Scale() const;

/// \brief Set the scale of the animation skeleton.
/// \param[in] _scale Scale for animation skeleton.
public: void SetScale(double _scale);

/// \brief Get whether the animation is interpolated on X.
/// \return True if interpolated on X.
public: bool InterpolateX() const;
public: SDFORMAT_WARN_UNUSED bool InterpolateX() const;

/// \brief Set whether the animation is interpolated on X.
/// \param[in] _interpolateX True to indicate interpolation on X.
Expand All @@ -110,15 +110,15 @@ namespace sdf

/// \brief Get the time in seconds when the pose should be reached.
/// \return Time in seconds.
public: double Time() const;
public: SDFORMAT_WARN_UNUSED double Time() const;

/// \brief Set the time in seconds when the pose should be reached.
/// \param[in] _time Time in seconds for the pose to be reached.
public: void SetTime(double _time);

/// \brief Get the pose to be reached.
/// \return Pose to be reached.
public: gz::math::Pose3d Pose() const;
public: SDFORMAT_WARN_UNUSED gz::math::Pose3d Pose() const;

/// \brief Set the pose to be reached.
/// \param[in] _pose Pose to be reached.
Expand All @@ -144,15 +144,15 @@ namespace sdf

/// \brief Get the unique id of the trajectory.
/// \return Trajectory id.
public: uint64_t Id() const;
public: SDFORMAT_WARN_UNUSED uint64_t Id() const;

/// \brief Set the ID of the trajectory.
/// \param[in] _id Trajectory Id.
public: void SetId(uint64_t _id);

/// \brief Get the type of the trajectory.
/// \return Type of the trajectory.
public: const std::string &Type() const;
public: SDFORMAT_WARN_UNUSED const std::string &Type() const;

/// \brief Set the animation type of the trajectory.
/// (should match the animation name).
Expand All @@ -161,22 +161,22 @@ namespace sdf

/// \brief Get the tension of the trajectory spline.
/// \return Tension of the trajectory spline.
public: double Tension() const;
public: SDFORMAT_WARN_UNUSED double Tension() const;

/// \brief Set the tension of trajectory spline.
/// \param[in] _tension Tension for the trajectory spline.
public: void SetTension(double _tension);

/// \brief Get the number of waypoints.
/// \return Number of waypoints.
public: uint64_t WaypointCount() const;
public: SDFORMAT_WARN_UNUSED uint64_t WaypointCount() const;

/// \brief Get a waypoint based on an index.
/// \param[in] _index Index of the waypoint. The index should be in the
/// range [0..WaypointCount()).
/// \return Pointer to the waypoint. Nullptr if the index does not exist.
/// \sa uint64_t WaypointCount() const
public: const Waypoint *WaypointByIndex(uint64_t _index) const;
public: SDFORMAT_WARN_UNUSED const Waypoint *WaypointByIndex(uint64_t _index) const;

/// \brief Add a new waypoint.
/// \param[in] _waypoint Waypoint to be added.
Expand All @@ -203,7 +203,7 @@ namespace sdf

/// \brief Get the name of the actor.
/// \return Name of the actor.
public: const std::string &Name() const;
public: SDFORMAT_WARN_UNUSED const std::string &Name() const;

/// \brief Set the name of the actor.
/// \param[in] _name Name of the actor.
Expand All @@ -214,7 +214,7 @@ namespace sdf
/// typically used to express the position and rotation of an actor in a
/// global coordinate frame.
/// \return The pose of the actor.
public: const gz::math::Pose3d &RawPose() const;
public: SDFORMAT_WARN_UNUSED const gz::math::Pose3d &RawPose() const;

/// \brief Set the pose of the actor.
/// \sa const gz::math::Pose3d &RawPose() const
Expand All @@ -225,61 +225,61 @@ namespace sdf
/// object's pose is expressed. An empty value indicates that the frame is
/// relative to the world frame.
/// \return The name of the pose relative-to frame.
public: const std::string &PoseRelativeTo() const;
public: SDFORMAT_WARN_UNUSED const std::string &PoseRelativeTo() const;

/// \brief Set the name of the coordinate frame relative to which this
/// object's pose is expressed. An empty value indicates that the frame is
/// relative to the world frame.
/// \param[in] _frame The name of the pose relative-to frame.
public: void SetPoseRelativeTo(const std::string &_frame);

/// \brief The path to the file where this element was loaded from.
/// \brief Get the path to the file where this element was loaded from.
/// \return Full path to the file on disk.
public: const std::string &FilePath() const;
public: SDFORMAT_WARN_UNUSED const std::string &FilePath() const;

/// \brief Set the path to the file where this element was loaded from.
/// \paramp[in] _filePath Full path to the file on disk.
public: void SetFilePath(const std::string &_filePath);

/// \brief Get the skin filename.
/// \return Constant skin filename.
public: const std::string &SkinFilename() const;
public: SDFORMAT_WARN_UNUSED const std::string &SkinFilename() const;

/// \brief Set the skin filename.
/// \param[in] _skinFilename Skin filename.
public: void SetSkinFilename(std::string _skinFilename);

/// \brief Get the skin scale.
/// \return Constant skin filename.
public: double SkinScale() const;
public: SDFORMAT_WARN_UNUSED double SkinScale() const;

/// \brief Set the skin scale.
/// \param[in] _skinScale Skin scale.
public: void SetSkinScale(double _skinScale);

/// \brief Get the number of animations.
/// \return Number of animations.
public: uint64_t AnimationCount() const;
public: SDFORMAT_WARN_UNUSED uint64_t AnimationCount() const;

/// \brief Get an animation based on an index.
/// \param[in] _index Index of the animation. The index should be in the
/// range [0..AnimationCount()).
/// \return Pointer to the animation. Nullptr if the index does not exist.
/// \sa uint64_t AnimationCount() const
public: const Animation *AnimationByIndex(uint64_t _index) const;
public: SDFORMAT_WARN_UNUSED const Animation *AnimationByIndex(uint64_t _index) const;

/// \brief Get whether an animation name exists.
/// \param[in] _name Name of the animation to check.
/// \return True if there exists an animation with the given name.
public: bool AnimationNameExists(const std::string &_name) const;
public: SDFORMAT_WARN_UNUSED bool AnimationNameExists(const std::string &_name) const;

/// \brief Add a new animation.
/// \param[in] _anim Animation to be added.
public: void AddAnimation(const Animation &_anim);

/// \brief Get whether the animation plays in loop.
/// \return True if the animation plays in loop.
public: bool ScriptLoop() const;
public: SDFORMAT_WARN_UNUSED bool ScriptLoop() const;

/// \brief Set whether the animation plays in loop.
/// \param[in] _scriptLoop True to indicate that the animation
Expand All @@ -288,15 +288,15 @@ namespace sdf

/// \brief Get the time (in seconds) of delay to start.
/// \return Time of delay to start.
public: double ScriptDelayStart() const;
public: SDFORMAT_WARN_UNUSED double ScriptDelayStart() const;

/// \brief Set the delay time to start.
/// \param[in] _scriptDelayStart Time of delay to start.
public: void SetScriptDelayStart(double _scriptDelayStart);

/// \brief Get whether the animation plays when simulation starts.
/// \return True if the animation plays when simulation starts.
public: bool ScriptAutoStart() const;
public: SDFORMAT_WARN_UNUSED bool ScriptAutoStart() const;

/// \brief Set whether the animation plays when simulation starts.
/// \param[in] _staticAutoStart True to indicate that the animation
Expand All @@ -305,61 +305,61 @@ namespace sdf

/// \brief Get the number of trajectories.
/// \return Number of trajectories.
public: uint64_t TrajectoryCount() const;
public: SDFORMAT_WARN_UNUSED uint64_t TrajectoryCount() const;

/// \brief Get a trajectory based on an index.
/// \param[in] _index Index of the trajectory. The index should be in the
/// range [0..TrajectoryCount()).
/// \return Pointer to the trajectory. Nullptr if the index does not exist.
/// \sa uint64_t TrajectoryCount() const
public: const Trajectory *TrajectoryByIndex(uint64_t _index) const;
public: SDFORMAT_WARN_UNUSED const Trajectory *TrajectoryByIndex(uint64_t _index) const;

/// \brief Get whether a trajectory id exists.
/// \param[in] _id Id of the trajectory to check.
/// \return True if there exists a trajectory with the given name.
public: bool TrajectoryIdExists(uint64_t _id) const;
public: SDFORMAT_WARN_UNUSED bool TrajectoryIdExists(uint64_t _id) const;

/// \brief Add a new trajectory.
/// \param[in] _traj Trajectory to be added.
public: void AddTrajectory(const Trajectory &_traj);

/// \brief Get the number of links.
/// \return Number of links.
public: uint64_t LinkCount() const;
public: SDFORMAT_WARN_UNUSED uint64_t LinkCount() const;

/// \brief Get a link based on an index.
/// \param[in] _index Index of the link. The index should be in the
/// range [0..LinkCount()).
/// \return Pointer to the link. Nullptr if the index does not exist.
/// \sa uint64_t LinkCount() const
public: const Link *LinkByIndex(uint64_t _index) const;
public: SDFORMAT_WARN_UNUSED const Link *LinkByIndex(uint64_t _index) const;

/// \brief Get whether a link name exists.
/// \param[in] _name Name of the link to check.
/// \return True if there exists a link with the given name.
public: bool LinkNameExists(const std::string &_name) const;
public: SDFORMAT_WARN_UNUSED bool LinkNameExists(const std::string &_name) const;

/// \brief Get the number of joints.
/// \return Number of joints.
public: uint64_t JointCount() const;
public: SDFORMAT_WARN_UNUSED uint64_t JointCount() const;

/// \brief Get a joint based on an index.
/// \param[in] _index Index of the joint. The index should be in the
/// range [0..JointCount()).
/// \return Pointer to the joint. Nullptr if the index does not exist.
/// \sa uint64_t JointCount() const
public: const Joint *JointByIndex(uint64_t _index) const;
public: SDFORMAT_WARN_UNUSED const Joint *JointByIndex(uint64_t _index) const;

/// \brief Get whether a joint name exists.
/// \param[in] _name Name of the joint to check.
/// \return True if there exists a joint with the given name.
public: bool JointNameExists(const std::string &_name) const;
public: SDFORMAT_WARN_UNUSED bool JointNameExists(const std::string &_name) const;

/// \brief Get a pointer to the SDF element that was used during
/// load.
/// \return SDF element pointer. The value will be nullptr if Load has
/// not been called.
public: sdf::ElementPtr Element() const;
public: SDFORMAT_WARN_UNUSED sdf::ElementPtr Element() const;

/// \brief Add a link to the actor.
/// \param[in] _link Link to add.
Expand All @@ -384,17 +384,17 @@ namespace sdf
/// Note that parameter passing functionality is not captured with this
/// function.
/// \return SDF element pointer with updated actor values.
public: sdf::ElementPtr ToElement() const;
public: SDFORMAT_WARN_UNUSED sdf::ElementPtr ToElement() const;

/// \brief Get the plugins attached to this object.
/// \return A vector of Plugin, which will be empty if there are no
/// plugins.
public: const sdf::Plugins &Plugins() const;
public: SDFORMAT_WARN_UNUSED const sdf::Plugins &Plugins() const;

/// \brief Get a mutable vector of plugins attached to this object.
/// \return A vector of Plugin, which will be empty if there are no
/// plugins.
public: sdf::Plugins &Plugins();
public: SDFORMAT_WARN_UNUSED sdf::Plugins &Plugins();

/// \brief Remove all plugins
public: void ClearPlugins();
Expand Down
11 changes: 11 additions & 0 deletions include/sdf/system_util.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,16 @@
*/
#define SDFORMAT_HIDDEN GZ_SDFORMAT_HIDDEN

/** \def SDFORMAT_WARN_UNUSED
* Use to represent "[[warn_if_unused]]" compiler directive on C++17 or above
*/

#if __cplusplus >= 201703L
#define SDFORMAT_WARN_UNUSED [[nodiscard]]
#else
#error "HELLO"
#define SDFORMAT_WARN_UNUSED
#endif
Copy link
Member

Choose a reason for hiding this comment

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

per #1358 (comment), we don't need to define this macro, we can use [[nodiscard]] directly in the header file. apologies for the back-and-forth

Copy link
Author

@Ryanf55 Ryanf55 Apr 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No worries. While I think I could do this to the model, this is perhaps a great candidate for using clang-tidy's automated refactoring capabilities. Although I don't have experience with it, perhaps someone at Gazebo does?

I don't really expect to have time to do it to the entire repo, at least manually. It's not quick work because I don't think you can find-replace or regex it.

In the PR, you can see I tried using the modernize-use-nodiscard builtin, but I could not get it working correctly.

Copy link
Member

Choose a reason for hiding this comment

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

I haven't configured clang-tidy before. I think it would be worth adding [[nodiscard]] to this class by hand and then comparing to what clang-tidy generates

Copy link
Member

Choose a reason for hiding this comment

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

I installed clang-tidy on a debian machine and built Ionic up to sdformat15 in a colcon workspace and then was able to apply the [[nodiscard]] attribute automatically using the following command after sourcing the workspace:

clang-tidy --fix -checks=-*,modernize-use-nodiscard src/sdformat/include/sdf/*.hh --  $(pkg-config --cflags sdformat15)

Copy link
Author

Choose a reason for hiding this comment

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

Fantastic. I'll go ahead and do the work manually and post a diff.

Copy link
Member

Choose a reason for hiding this comment

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

untested changes to all header files in 1d96a58

I'm pretty sure it pushes some lines over 80 characters, which would need to be fixed


// SDF_VISIBLE_HH_
#endif
Loading