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

Change TF2Error names to be a bit more descriptive. #349

Merged
merged 1 commit into from
Sep 16, 2021

Conversation

clalancette
Copy link
Contributor

The driving reason behind this is to avoid the NO_ERROR macro
on Windows. We can't totally get around the problem right now,
but we can deprecate the old names and switch to new ones.
After Galactic, we can remove the old names and get rid of the
problem completely.

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

This obsoletes #247 (which I'll go ahead and close out).

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

You could have a message with the deprecation, like "use TF2_X instead", but otherwise lgtm.

tf2/include/tf2/exceptions.h Show resolved Hide resolved
@clalancette
Copy link
Contributor Author

You could have a message with the deprecation, like "use TF2_X instead", but otherwise lgtm.

Yeah, good point. I added that in d711136 . I'll run CI now:

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

@clalancette
Copy link
Contributor Author

The Windows warnings are known, and are being addressed elsewhere.

The macOS warnings are annoying. They are saying that [[deprecation]] on an enum value is a C++17-only feature, and since everything here is currently C++-14, we get the warning.

One way out of this conundrum is to wait an see what happens with ros-infrastructure/rep#291 . If we decide to switch to C++17, then we can just do that and use the deprecation as-is. The other thing we can possibly do is to deprecate the enum entirely, and come up with a new enum with new names. I'm going to hold off on this one for now.

@wjwwood
Copy link
Member

wjwwood commented Nov 30, 2020

on an enum value is a C++17-only feature, and since everything here is currently C++-14, we get the warning.

Oh yeah, that came up in the past I think.

@clalancette
Copy link
Contributor Author

All right, since we can now switch to C++17, I've done that in this PR. Let's see what CI looks like now:

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

@clalancette
Copy link
Contributor Author

And of course now Windows is annoyed at C++17 changes. Sigh. OK, this is (still) a puzzle for another day.

@clalancette
Copy link
Contributor Author

clalancette commented Feb 4, 2021

Given changes elsewhere, here is another run on Windows to see if it is any happier:

  • Windows Build Status

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

LGTM, this may generate some deprecation warnings on some other packages which it would be great to track. I think we should run CI with --packages-above-and-dependencies tf2

The driving reason behind this is to avoid the NO_ERROR macro
on Windows.  We can't totally get around the problem right now,
but we can deprecate the old names and switch to new ones.
After Galactic, we can remove the old names and get rid of the
problem completely.

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

Instead of using C++17, which causes some downstream breakage, I think the simpler thing to do here is to ignore the warning that clang throws on macOS. This whole thing will eventually go away (for I-Turtle) anyway, so I think that is a reasonable enough thing to do. The latest commit does exactly that.

LGTM, this may generate some deprecation warnings on some other packages which it would be great to track.

My earlier CI attempts show that it doesn't, at least not in the core. There may be some breakage outside of the core, but that's why it is deprecated, not removed :).

run CI with --packages-above-and-dependencies tf2

Yep, definitely. Will run new CI with the latest commit and everything above tf2.

@clalancette
Copy link
Contributor Author

CI:

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

@clalancette
Copy link
Contributor Author

CI is green here, so I'm going to go ahead and merge this. Thanks for the reviews.

@ahcorde If you have time, could you please review the release note I added for this at ros2/ros2_documentation#1957 ? Thanks.

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.

3 participants