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

Remove compiler warnings #44

Merged
merged 2 commits into from
Feb 21, 2024
Merged

Conversation

Ryanf55
Copy link
Contributor

@Ryanf55 Ryanf55 commented Feb 19, 2024

This removes the compiler warnings. A few of the functions had unused epsilon values.

In getClosestPoint, I used the epsilon value from the function in a place that seems correct, however they were different by an order of magnitude. If you think that's wrong, let me know.

Finally, the pi-wrapping functions were converted to inline because they were defined and used in different translation units:

/__w/terrain-navigation/terrain-navigation/src/ethz-asl/terrain-navigation/terrain_navigation/include/terrain_navigation/path_segment.h:61:13: warning: ‘void wrap_pi(double&)’ defined but not used [-Wunused-function]
   61 | static void wrap_pi(double &angle) {

Using inline stops the compiler from caring about that. I looked briefly to see if there was any pi-wrapping utilities in eigen, but could not find any. There are numerous optimizations that can be made to the current algorithm, but I'm not going to bother until we have benchmarking set up.

Signed-off-by: Ryan Friedman <[email protected]>
Copy link
Member

@Jaeyoung-Lim Jaeyoung-Lim left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

Signed-off-by: Ryan Friedman <[email protected]>
@Jaeyoung-Lim Jaeyoung-Lim merged commit ff8f038 into ethz-asl:ros2 Feb 21, 2024
3 checks passed
@Ryanf55
Copy link
Contributor Author

Ryanf55 commented Feb 21, 2024

All the compile warning are now removed. Would you like me to issue a PR to enforce we don't have regressions in CI?

@Ryanf55 Ryanf55 deleted the remove-warnings branch February 21, 2024 13:17
@Jaeyoung-Lim
Copy link
Member

@Ryanf55 Yes please!

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