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

Use four 32-bit signed parts to represent the client id #28

Closed
wants to merge 1 commit into from

Conversation

esteve
Copy link
Member

@esteve esteve commented Oct 27, 2015

This PR replaces unsigned 64-bit integers with signed 32-bit integers to ensure portability.

Connects to ros2/rmw_connext#31

@esteve esteve added the in progress Actively being worked on (Kanban column) label Oct 27, 2015
@tfoote tfoote closed this Nov 17, 2015
@tfoote tfoote removed the in progress Actively being worked on (Kanban column) label Nov 17, 2015
@mikaelarguedas
Copy link
Member

mikaelarguedas commented Aug 8, 2017

@tfoote or @ros2/team can you confirm that this change has been considered unnecessary because the RTPS specification states that the GUID has to fit on 16 octets ? (12 for the prefix and 4 for the entity) Relevant sections of the spec: 8.2.1.2 and 8.4.2.1

Or because we assume the machines can emulate 64 bits by using 2 32-bit registers internally without requiring the application to explicitly split them into 32-bit variables?

@tfoote
Copy link

tfoote commented Aug 8, 2017

I think this was closed during a meeting when I was logged in. But I believe that this was closed with the expectation that 64bit integers would work.

The logic was related to connext's query parser: ros2/rmw_opensplice#88 (comment) but I think we determined that we didn't need to use that? I'm not sure though.

@mikaelarguedas
Copy link
Member

sounds good thanks! deleting this branch then. If anybody has more insights feel free to comment here

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.

None yet

3 participants