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

Convert ROSIDL char to IDL char #698

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from

Conversation

ijnek
Copy link

@ijnek ijnek commented Aug 8, 2022

A char is clearly defined in IDL, and the conversion from IDL->python, IDL->C++, IDL->C for a char all seems to exist, yet currently char is converted to uint8 in rosidl_adapter. Is there a reason for this conversion?

Currently:

  • rosidl_adapter: char -> uint8 then,
  • C / C++: uint8 -> uint8_t
  • Python: uint8 -> int

But to me what makes sense, would be:

  • rosidl_adapter: char -> char then,
  • C / C++: char -> char (char8_t in c++20, but uint8_t if we want to really specify 8 bit usage)
  • Python: char -> str with length 1

The design docs will have to be updated if these change.


UPDATE:

I just realised that char has been deprecated since ROS 1, and the conversion is somewhat justified - quoting ROS 1 msg:

Deprecated:

char: deprecated alias for uint8
byte: deprecated alias for int8

and in ROS2 design, it mentions:

While byte and char are deprecated in ROS 1 they are still part of this definition to ease migration.

Is this deprecation ongoing, and would it be possible to add a deprecation notice somewhere during compilation?

Separate point - byte doesn't seem like an alias for int8 as explained in the quotes above, it maps to unsigned char for C++...

Signed-off-by: Kenji Brameld <[email protected]>
@ijnek
Copy link
Author

ijnek commented Aug 26, 2022

Anyone have thoughts on this?

@AlexisTM
Copy link
Contributor

C++ related:
Just like std::byte, I wouldn't use char8_t as while semantically better, they are considered harmful by many committee members.
char is also to be avoided because it is implementation defined and can be signed or unsigned depending on the platform, creating hard bugs.

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