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 warning #163

Merged
merged 3 commits into from
Jun 4, 2019
Merged

Fix armhf warning #163

merged 3 commits into from
Jun 4, 2019

Conversation

emersonknapp
Copy link
Contributor

@emersonknapp emersonknapp commented Jun 3, 2019

Related to ros2/ros2#721

Fixes these warnings https://ci.ros2.org/view/All/job/test_ci_linux-armhf/8/warnings23Result/package.-1643903456/

On armhf, size_t is 32 bits, so gcc raises warning for this line "comparison is always true due to limited range of data type"

Signed-off-by: Emerson Knapp eknapp@amazon.com

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@@ -33,6 +33,10 @@ extern "C"
// RCUTILS_REPORT_ERROR_HANDLING_ERRORS and RCUTILS_WARN_ON_TRUNCATION are set in the header below
#include "./error_handling_helpers.h"

#ifndef SIZE_MAX
Copy link
Contributor

Choose a reason for hiding this comment

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

include <stdint.h> for SIZE_MAX? On what platforms is it undefined?

Copy link
Contributor Author

@emersonknapp emersonknapp Jun 3, 2019

Choose a reason for hiding this comment

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

include <stdint.h> for SIZE_MAX

yes, good catch

On what platforms is it undefined?

It's a C99 definition, I think the only place we don't get C99 is on Windows.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like VS 2017 and 2019 has SIZE_MAX in <limits.h>. https://docs.microsoft.com/en-us/cpp/c-runtime-library/data-type-constants?view=vs-2017 . Do you know if the "C Run-Time Library" is available when building C++ code on windows? If so, maybe the ifndef can be replaced with another include.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know offhand if it's available, but I can try a CI run on Windows without the ifndef to see if it's okay that way

Emerson Knapp added 2 commits June 3, 2019 15:40
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp
Copy link
Contributor Author

emersonknapp commented Jun 4, 2019

windows Build Status
linux-armhf Build Status (compare against Build Status)

Note: that armhf build is strict improvement over baseline, removes rcutils warnings

@dirk-thomas dirk-thomas merged commit e49e277 into ros2:master Jun 4, 2019
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