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

Fix armhf build warnings #372

Merged
merged 1 commit into from
Jun 21, 2019
Merged

Conversation

prajakta-gokhale
Copy link

@prajakta-gokhale prajakta-gokhale commented Jun 19, 2019

Fix armhf build warnings caused by incorrect format specifiers.

Build warnings seen here.
Related to ros2/ros2#721.

Signed-off-by: Prajakta Gokhale prajaktg@amazon.com

@prajakta-gokhale
Copy link
Author

@emersonknapp - please run this CI job
Gist: https://gist.githubusercontent.com/prajakta-gokhale/82ce46e08e7e70a203500381ee575d68/raw/96b707c5c5a30bbd3991db3e1143580d9df501af/ros2.repos
BUILD args: --packages-up-to pendulum_control
TEST args: --packages-select pendulum_control
Job: ci_launcher

@emersonknapp
Copy link
Contributor

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

@emersonknapp
Copy link
Contributor

ah - the printf specifiers are tricky - we may need to do an explicit cast on them, if they're using the uintN_t types

@prajakta-gokhale
Copy link
Author

ah - the printf specifiers are tricky - we may need to do an explicit cast on them, if they're using the uintN_t types

Right. I'll change them to explicit typecasts because changing format specifiers isn't gonna work on all platforms.

@emersonknapp
Copy link
Contributor

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

printf("Mean latency: %f ns\n", msg->mean_latency);
printf("Min latency: %lu ns\n", msg->min_latency);
printf("Max latency: %lu ns\n", msg->max_latency);
printf("Min latency: %llu ns\n", (long long) msg->min_latency);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing these C-style casts, I'd much prefer if we used the PRId64 series of macros. That will ensure it will work on all platforms without casts, and is the solution we've used elsewhere in the codebase. The same goes for the changes below.

Copy link
Author

Choose a reason for hiding this comment

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

Changed in next revision.

Fix armhf build warnings caused by incompatible format specifiers.

Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Looks good to me with green CI.

@emersonknapp
Copy link
Contributor

emersonknapp commented Jun 20, 2019

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

Edit: rebuilding "Linux" because the build machine seems to have had a network hiccup. Linux-armhf is strict improvement over upstream.

@emersonknapp
Copy link
Contributor

@clalancette good to merge?

@clalancette clalancette merged commit b7581d1 into ros2:master Jun 21, 2019
@emersonknapp emersonknapp deleted the fix-armhf-warnings branch June 21, 2019 17:02
cottsay pushed a commit that referenced this pull request Oct 11, 2019
Fix armhf build warnings caused by incompatible format specifiers.

Signed-off-by: Prajakta Gokhale <prajaktg@amazon.com>
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.

4 participants