Skip to content

Minor code cleanups in tf2#467

Merged
clalancette merged 11 commits intoros2from
clalancette/tf2-code-cleanups
Oct 18, 2021
Merged

Minor code cleanups in tf2#467
clalancette merged 11 commits intoros2from
clalancette/tf2-code-cleanups

Conversation

@clalancette
Copy link
Copy Markdown
Contributor

This series of patches aims to do a bit of minor cleanup on the code in tf2. There should be no functional change with this in place. The changes include:

  1. Removing a long-deprecated function overload
  2. Removal of an unnecessary internal function
  3. Include-what-you-use in a few places
  4. Use foo.empty() instead of foo == ""
  5. Use std::make_shared where possible
  6. Replace NULL with nullptr
  7. Remove unused code
  8. Switch to C++ style casts
  9. Move some functions from a header file to the C++ file

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
This is more efficient.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
It just makes more sense.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
This should improve compile times for downstream projects.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Copy Markdown
Contributor Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Copy link
Copy Markdown
Member

@aprotyas aprotyas left a comment

Choose a reason for hiding this comment

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

LGTM. On a similar note, what about common linter (and cppcheck) tests on this package?

I noticed this comment in the CMakeLists.txt file:

# TODO(clalancette) enable linters once https://github.com/ament/ament_lint/pull/238 lands

It looks like ament/ament_lint#238 has been closed and replaced with the (now merged) ament/ament_lint#299 though. Thoughts? I could put this in a separate PR akin to #464.

@clalancette
Copy link
Copy Markdown
Contributor Author

It looks like ament/ament_lint#238 has been closed and replaced with the (now merged) ament/ament_lint#299 though. Thoughts? I could put this in a separate PR akin to #464.

Oh, I didn't even check. That would be great @aprotyas !

@clalancette
Copy link
Copy Markdown
Contributor Author

CI is green, and this has an approval. I'm going to go ahead and merge, thanks for the review!

@clalancette clalancette merged commit 16562ce into ros2 Oct 18, 2021
@delete-merged-branch delete-merged-branch bot deleted the clalancette/tf2-code-cleanups branch October 18, 2021 12:26
aprotyas added a commit that referenced this pull request Dec 18, 2021
As discussed in #467:

- Enables a subset of relevant linter tests from `ament_lint_common` in `tf2`.
- Excludes LinearMath headers (include/tf2/LinearMath) from being linted.
- Fixes line-length formatting issues in a few source files, so that there are no lint failures in the current state.

Note that individual linters had to be invoked rather than use `ament_lint_auto` because the LinearMath headers should not have linters run on them - see #258 (comment) for more information - and because `ament_lint_auto` does not provide a general way of providing file exclusion lists.

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants