Skip to content

Cleanups in buffer_core.cpp.#301

Merged
clalancette merged 1 commit intoros2from
tf2_buffer_core_cleanup
Sep 4, 2020
Merged

Cleanups in buffer_core.cpp.#301
clalancette merged 1 commit intoros2from
tf2_buffer_core_cleanup

Conversation

@clalancette
Copy link
Copy Markdown
Contributor

This does a few things:

  1. Removes commented out code. This code has been commented
    out for a decade, so I think it is safe to say nobody is
    using it.
  2. Uses size_t everywhere we are calling vector.size()
  3. Ensures all member variables are suffixed with _
  4. Removes asserts and replaces them with exceptions.
  5. Gets rid of redundant branches for error cases and
    uses just one switch statement for all possible return values.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

@clalancette
Copy link
Copy Markdown
Contributor Author

clalancette commented Aug 6, 2020

CI:

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

@clalancette clalancette force-pushed the tf2_buffer_core_cleanup branch 3 times, most recently from df0ba86 to c8ff1f5 Compare August 10, 2020 12:26
default:
CONSOLE_BRIDGE_logError("Unknown error code: %d", retval);
assert(0);
throw std::runtime_error("Unknown error code: " + std::to_string(static_cast<int>(retval)));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is specifically an assert not an exception as it's not something that can or should be hit without a coding error and then can be compiled out for production. And isn't something that's recoverable so the developers shouldn't be trying to catch it.

When this was ported it used a ROS_ASSERT before. And the equivalent functionality was proposed to be ported here but that was stopped because it was a port and not a reimplementation: ros2/rcutils#112

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

All right. I've put this back to an assert. I don't think it is in the purview of a library to assert, but I can live without it for now for the rest of the cleanups.

This does a few things:
1.  Removes commented out code.  This code has been commented
out for a decade, so I think it is safe to say nobody is
using it.
2.  Uses size_t everywhere we are calling vector.size()
3.  Ensures all member variables are suffixed with _
4.  Removes asserts and replaces them with exceptions.
5.  Gets rid of redundant branches for error cases and
uses just one switch statement for all possible return values.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette clalancette force-pushed the tf2_buffer_core_cleanup branch from 847d768 to 593b9de Compare September 1, 2020 12:46
@clalancette
Copy link
Copy Markdown
Contributor Author

@ros-pull-request-builder retest this please

@clalancette
Copy link
Copy Markdown
Contributor Author

clalancette commented Sep 1, 2020

CI:

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

@clalancette clalancette merged commit cab7c8c into ros2 Sep 4, 2020
@delete-merged-branch delete-merged-branch bot deleted the tf2_buffer_core_cleanup branch September 4, 2020 19:34
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