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

Compile flag fixes #114

Closed
wants to merge 1 commit into from
Closed

Conversation

jpsamper2009
Copy link
Contributor

@jpsamper2009 jpsamper2009 commented Aug 9, 2018

Description

  • Courtesy of @serge-nikulin
  • This PR fixes warning generated when building with:
-Wall
-Wextra
-pedantic
-Wcast-align
-Wunused
-Wconversion
-Wsign-conversion
-Wdouble-promotion
-fvisibility=hidden

@esteve esteve added the in review Waiting for review (Kanban column) label Aug 9, 2018
@esteve esteve removed the in review Waiting for review (Kanban column) label Aug 20, 2018
@mikaelarguedas mikaelarguedas added the in review Waiting for review (Kanban column) label Aug 20, 2018
@@ -30,7 +30,7 @@ bool rcutils_cli_option_exist(char ** begin, char ** end, const char * option)
char * rcutils_cli_get_option(char ** begin, char ** end, const char * option)
{
size_t idx = 0;
size_t end_idx = end - begin;
size_t end_idx = (size_t)(end - begin);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking around a bit, I think the basic problem here is that taking the difference between two pointers should be represented by ptrdiff_t, not size_t (in theory, the difference can be negative: https://bytes.com/topic/c/answers/460624-subtracting-pointers-type-use-store-result). While changing to this, you'll probably also have to change the type of idx to match.

@@ -195,7 +195,7 @@ format_error_string(void)
if (!__rcutils_error_is_set(__rcutils_error_state)) {
return;
}
size_t bytes_it_would_have_written = snprintf(
size_t bytes_it_would_have_written = (size_t)snprintf(
Copy link
Contributor

Choose a reason for hiding this comment

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

There's actually a few problems here. snprintf is defined to return an integer, because the return value can be negative on failure. So the return value should be stored in an integer. After that, bytes_it_would_have_written should be checked for negative; if it failed, we definitely don't want to continue on and pass bogus sizes around as unsigned. Finally, we would need a cast from int to size_t while passing bytes_it_would_have_written into allocator.allocate() and the lower-down snprintf to avoid the warnings.

@@ -45,7 +45,7 @@ rcutils_format_string_limit(
va_list args2;
va_copy(args2, args1);
// first calculate the output string
size_t bytes_to_be_written = rcutils_vsnprintf(NULL, 0, format_string, args1);
size_t bytes_to_be_written = (size_t)rcutils_vsnprintf(NULL, 0, format_string, args1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Like in error_handling.c, the return value of rcutils_vsnprintf is actually int, not size_t, since it can fail. The same comments apply about storing in an int, checking for negative, and then casting where it is used.

@clalancette clalancette added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Jan 3, 2019
@cottsay cottsay added ready Work is about to start (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Feb 14, 2019
@mabelzhang mabelzhang removed the ready Work is about to start (Kanban column) label Sep 19, 2019
@jpsamper2009
Copy link
Contributor Author

This PR was a for a much older version of the code. I'll close it and readdress with a newer version (if the issues still exist).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants