Skip to content

Conversation

InvincibleRMC
Copy link
Contributor

@InvincibleRMC InvincibleRMC commented Jul 29, 2025

Description

Resolves TODO to use std::byte. Downstream users will need to update if they use IDL bytes. The message generation documentation would also need to be updated. Will also likely need a ROS Discourse post.

Is this user-facing behavior change?

Yes users will need to switch to using std::byte rather than the existing conversion like int.

Did you use Generative AI?

No

Additional Information

Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
Copy link
Contributor

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm with green CI. i would like to have another review.

@christophebedard
Copy link
Member

Note that this is a breaking change.

@InvincibleRMC
Copy link
Contributor Author

InvincibleRMC commented Jul 31, 2025

While working on #882 Found a big use for std::byte simplifying to msg__traits.hpp. Will probably split it out of #882.

Edit: My cool simplification will need to wait for C++ 20 and the addition of char8_t since it needs a way to distinguish between unsigned char and uint8_t. Or I guess hypothetically we could add a custom roschar8_t until C++ 20 similar to BoundedVector.

@InvincibleRMC
Copy link
Contributor Author

Note that this is a breaking change.

If/When this gets merged I can help fix downstream bugs.

@fujitatomoya fujitatomoya requested a review from ahcorde August 7, 2025 02:36
@fujitatomoya
Copy link
Contributor

Pulls: #881
Gist: https://gist.githubusercontent.com/fujitatomoya/c0b962717673b135a1a582f9282c0413/raw/96d7a6596347cf690185cc4c41a1172af1b89aec/ros2.repos
BUILD args: --packages-above-and-dependencies rosidl_generator_cpp rosidl_generator_tests rosidl_runtime_cpp rosidl_typesupport_introspection_tests
TEST args: --packages-above rosidl_generator_cpp rosidl_generator_tests rosidl_runtime_cpp rosidl_typesupport_introspection_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16671

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@InvincibleRMC
Copy link
Contributor Author

Unsurprisingly this causes errors in rclcpp and presumably other packages further downstream. Should I try and add things to mitigate the pain of migration e.j. add in way to set bytes from the old unsigned char type with a deprecation warning and/or add a retrieval method of the bytes attributes which is the old unsigned char type. Or is the rip off bandaid approach fine?

@christophebedard
Copy link
Member

We usually avoid breaking changes and try to deprecate something first in one release and then remove it in the next release, see https://docs.ros.org/en/rolling/The-ROS2-Project/Contributing/Developer-Guide.html#deprecation-strategy. That's what I really meant with my "breaking change" comment above; sorry I wasn't clear.

So yeah, we should try to support setting/getting an unsigned char while keeping std::byte underneath and let users migrate to it, which would let users migrate to std::byte without breaking their packages.

@InvincibleRMC
Copy link
Contributor Author

InvincibleRMC commented Aug 8, 2025

The following works for everything except compound assignments e.j. += those overloads would need to be added. Something similar for adding support for containers of byte would also need to be implemented. Feel free to give feedback or if there is a more clever approach let me know.

EDIT: Updated so size matches byte size

#include <cstddef>
#include <iostream>

struct DeprecatedHelperByte {
    std::byte storage_{};

    [[deprecated("Assign std::byte instead")]]
    operator unsigned char() const noexcept {
        std::cout << "getter called char\n";
        return static_cast<unsigned char>(storage_);
    }

    operator std::byte() const noexcept {
        std::cout << "getter called byte\n";
        return storage_;
    }

    DeprecatedHelperByte& operator=(std::byte b) noexcept {
        std::cout << "setter called byte\n";
        storage_ = b;
        return *this;
    }

    [[deprecated("Assign std::byte instead")]]
    DeprecatedHelperByte& operator=(unsigned char b) noexcept {
        std::cout << "setter called char\n";
        storage_ = std::byte{b};
        return *this;
    }
};

struct Foo {
    DeprecatedHelperByte data{};
};

int main() {
    Foo f;

    f.data = std::byte{1};
    unsigned char b = f.data;
    f.data = 1;
    std::byte c = f.data;

    std::cout << "\nSize of Foo: " << sizeof(Foo) << '\n';
}

Signed-off-by: Michael Carlstrom <[email protected]>
@InvincibleRMC
Copy link
Contributor Author

Hmm while this works great it would need a specialization for all the containers (vector, array, bounded_sequence) ROS uses.

Will take into a look into just adding conversion methods.

@InvincibleRMC InvincibleRMC marked this pull request as draft August 10, 2025 05:29
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
Signed-off-by: Michael Carlstrom <[email protected]>
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