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

Map ROS char into C signed char #211

Merged
merged 6 commits into from
Apr 6, 2017
Merged

Map ROS char into C signed char #211

merged 6 commits into from
Apr 6, 2017

Conversation

gerkey
Copy link
Member

@gerkey gerkey commented Apr 5, 2017

The C standard leaves it to the compiler implementor to decide whether a char is signed or unsigned:

As a result, it's not portable to assign a negative value to a char and then test the result. This came up when building on aarch64:

This PR changes the mapping of the ROS char to be a signed char in C. Requires ros2/rmw_fastrtps#99.

I'll also do a PR to update the design doc to specify the new mapping, but these code changes can be reviewed independently of that.

For a smaller test case, consider this program:

#include <stdio.h>

int main(void)
{
  char c1 = 66;
  char c2 = -66;

  if (c1 == 66)
    printf("positive pass\n");
  else
    printf("positive fail\n");

  if (c2 == -66)
    printf("negative pass\n");
  else
    printf("negative fail\n");
  return 0;
}

On linux/x86 it looks good:

$ gcc -Wtype-limits -o foo foo.c
$ ./foo
positive pass
negative pass

But on linux/aarch64 not so much:

$ gcc -Wall -Wtype-limits -o foo foo.c
foo.c: In function ‘main’:
foo.c:13:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
   if (c2 == -66)
          ^
$ ./foo
positive pass
negative fail

@gerkey gerkey added in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 5, 2017
@mikaelarguedas
Copy link
Member

This looks to me like a good reason to revive the char/byte usefulness in our IDL format.

The design doc defines:

char:
an integer value in the following interval [-128, 127]

I'm not convinced that changing the tests to stop testing the entire range of the char type is a long term solution. Maybe reviving #190 and the associated Pull Requests will be more adequate.

If we are looking for a short term solution I think that we should explicitly assign this to a signed char to comply with our spec and compiler on ARM.

@mikaelarguedas
Copy link
Member

Thanks for iterating on this. Can c6e45ca be reverted now that signed char is used ?

Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

Please also create a corresponding PR for the design article which specifies the type mapping to be in sync.

@@ -1,7 +1,7 @@
bool def_bool_1 true
bool def_bool_2 false
byte def_byte 66
char def_char -66
char def_char 67
Copy link
Member

Choose a reason for hiding this comment

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

This should be reverted.

Same below for test.

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -166,7 +166,7 @@ def primitive_value_to_c(type_, value):
if type_ == 'bool':
return 'true' if value else 'false'

if type_ in ['byte', 'char', 'int8', 'int16', 'int32', 'int64']:
if type_ in ['byte', 'char', 'signed char', 'int8', 'int16', 'int32', 'int64']:
Copy link
Member

Choose a reason for hiding this comment

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

signed char is not a valid type name. Valid type names are only the keys of the MSG_TYPE_TO_C dict.

Copy link
Member Author

Choose a reason for hiding this comment

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

@gerkey
Copy link
Member Author

gerkey commented Apr 5, 2017

@gerkey gerkey added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Apr 5, 2017
@gerkey
Copy link
Member Author

gerkey commented Apr 5, 2017

@gerkey
Copy link
Member Author

gerkey commented Apr 5, 2017

@gerkey gerkey changed the title Don't test assigning a negative value to a char because it's not portable Map ROS char into C signed char Apr 6, 2017
@gerkey gerkey added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 6, 2017
@gerkey
Copy link
Member Author

gerkey commented Apr 6, 2017

Now that aarch64 is happy, here's full CI:

  • Linux:
    • Build Status
  • Linux aarch64:
    • Build Status
  • macOS:
    • Build Status
  • Windows:
    • Build Status

Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

lgtm once CI passed

@gerkey gerkey merged commit 4343c3f into master Apr 6, 2017
@gerkey gerkey deleted the fix_test branch April 6, 2017 20:00
@gerkey gerkey removed the in review Waiting for review (Kanban column) label Apr 6, 2017
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