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 a list instead of a set for node names list #112

Merged
merged 1 commit into from
Mar 11, 2020

Conversation

emersonknapp
Copy link
Contributor

@emersonknapp emersonknapp commented Mar 11, 2020

The other rmw implementations currently return all node names, even if there are duplicates. This behavior allows us to reason about there being duplicate nodes in the system and warn the user about it, even if we can't solve it. Using the std::set only masks this potentially confusing case.

Unblocks ros2/ros2cli#463
Related to ros2/ros2cli#453
Related to https://github.com/ros-tooling/aws-roadmap/issues/196

Signed-off-by: Emerson Knapp emerson.b.knapp@gmail.com

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Copy link
Contributor Author

@eboasson do you have an opinion on this decision?

@rotu
Copy link
Collaborator

rotu commented Mar 11, 2020

This seems like an ideal place to use std::multiset instead of set or vector. This has the benefit of returning the items in sorted order.

@eboasson
Copy link
Collaborator

@emersonknapp my opinion is that there should not be any gratuitous changes in behaviour when switching RMW layers. That it coughs up a set is simply a mistake on my part.

I agree with @rotu that returning them in sorted order would generally be a nice touch, but after a bit of digging:

  • the interface description doesn't mention anything about order;
  • the Fast-RTPS implementation returns them in GUID order (no matter the exact definition of the ordering defined on GUIDs);
  • the OpenSplice one in the order in which the read call on the DCPSParticipant topic returns time, and that's roughly equivalent to Fast-RTPS's behaviour;
  • the Connext one in the order in which they are returned by get_discovered_participants which itself yields in them in unspecified order.

So in my view no-one will be able to depend on them being returned in sorted order and without updating all RMW implementations and updating the interface specification, there is simply no benefit to returning them in sorted order.

@eboasson eboasson merged commit 8e8b1ff into ros2:master Mar 11, 2020
@emersonknapp
Copy link
Contributor Author

Thanks for the quick response - I agree that since the API doesn't specify an ordering and the other rmw implementations don't currently do it, it's not worth the overhead 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