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.

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Ryanf55
Copy link

@Ryanf55 Ryanf55 commented Jan 30, 2024

🎉 New feature

Relates to #1358

Summary

  • Add a macro to start using nodiscard as recommended in 1358.
  • Demonstrate usage of the macro in Actor.hh
  • If this PR is the right approach, then a future PR (or PR's) can fold this across to the rest of the repo.

In theory, clang-tidy should be able to automatically apply this across the whole repo, but clang-tidy doesn't seem to work with this repo. If anyone knows how to use it, that would be much preferable because automated code refactors are much less error-prone and quicker than me doing it manually.

Test it

  • Compilation is the only thing needed
  • If you want, try calling a getter in an application, and ignoring the return value. If you compile with C++17, it should warn.
  • Same as above, but if you compile with C++14, you should not expect a warning.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

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.

Signed-off-by: Ryan Friedman <[email protected]>
#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

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

@Ryanf55 Ryanf55 marked this pull request as draft April 23, 2024 00:16
@azeey azeey removed their request for review June 18, 2024 19:38
@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 29, 2024
@azeey
Copy link
Collaborator

azeey commented Aug 19, 2024

@Ryanf55, @scpeters we don't have much time left before the code freeze. Is this likely to make it by next Wednesday (08/28)?

@azeey azeey added Breaking change Breaks API, ABI or behavior. Must target unstable version. and removed beta Targeting beta release of upcoming collection 🏛️ ionic Gazebo Ionic labels Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking change Breaks API, ABI or behavior. Must target unstable version.
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

3 participants