Switch to std::map for TopicTypeMap #546

ahcorde · 2025-03-14T12:39:05Z

I open this PR just for visibility, it's related with this PR #536 and in particular with this comment #536 (comment)

Signed-off-by: Alejandro Hernandez Cordero <[email protected]>

Yadunund

@clalancette I'm sure had a good reason to introduce tsl::ordered_map in #536. Do you recall which tests were failing without it? Could it have been that the requirements were to return topic names in alphabetical order and not insertion order and that the failing test actually added topics in alphabetical order?

Yadunund

We should also delete

https://github.com/ros2/rmw_zenoh/blob/rolling/rmw_zenoh_cpp/src/detail/ordered_map.hpp and https://github.com/ros2/rmw_zenoh/blob/rolling/rmw_zenoh_cpp/src/detail/ordered_hash.hpp

And update CI to not reference those file

rmw_zenoh/.github/workflows/style.yaml

Lines 22 to 24 in caaedde

    
             run: /ros_entrypoint.sh ament_uncrustify rmw_zenoh_cpp/ --exclude rmw_zenoh_cpp/src/detail/ordered_hash.hpp rmw_zenoh_cpp/src/detail/ordered_map.hpp 
        
           - name: cpplint 
        
             run: /ros_entrypoint.sh ament_cpplint rmw_zenoh_cpp/ --exclude rmw_zenoh_cpp/src/detail/ordered_hash.hpp rmw_zenoh_cpp/src/detail/ordered_map.hpp

Yadunund · 2025-03-18T16:57:13Z

rmw_zenoh_cpp/src/detail/graph_cache.hpp

@@ -85,7 +85,7 @@ struct GraphNode
  // Map topic name to TopicTypeMap
  // This uses a map that remembers insertion order because some parts of the client libraries
  // expect that these are returned in the order that they were created.
-  using TopicMap = tsl::ordered_map<std::string, TopicTypeMap>;
+  using TopicMap = std::map<std::string, TopicTypeMap>;


Update documentation above.

…pic_map

Signed-off-by: Alejandro Hernandez Cordero <[email protected]>

clalancette · 2025-03-20T12:57:09Z

@clalancette I'm sure had a good reason to introduce tsl::ordered_map in #536. Do you recall which tests were failing without it? Could it have been that the requirements were to return topic names in alphabetical order and not insertion order and that the failing test actually added topics in alphabetical order?

Oof, this is a tough one. I made a big mistake in PR #250 in that I did not specify which tests were failing. And there definitely were failing tests at that point that adding the ordered map fixed.

So I went ahead and rebuilt a workspace with this PR in it, and ran all tests. With this in place, every test passed. So whatever it was that was failing before, was fixed in a different way. Thus, I think we can go forward with this change (with one change, which I'll put in a review).

clalancette

We should also remove the MIT license from the package.xml, as it is no longer needed.

Signed-off-by: Alejandro Hernandez Cordero <[email protected]>

clalancette · 2025-03-20T14:43:11Z

rmw_zenoh_cpp/CMakeLists.txt

@@ -94,8 +94,6 @@ if(BUILD_TESTING)
  find_package(ament_cmake_uncrustify REQUIRED)
  find_package(ament_cmake_xmllint REQUIRED)

-  set(_linter_excludes src/detail/ordered_hash.hpp src/detail/ordered_map.hpp)
-
  ament_copyright(EXCLUDE src/detail/simplified_xxhash3.cpp src/detail/simplified_xxhash3.hpp)
  ament_cpplint(EXCLUDE ${_linter_excludes})


Sorry, I just noticed this one. Since we are no longer setting _linter_excludes, this should be:

Suggested change

ament_cpplint(EXCLUDE ${_linter_excludes})

ament_cpplint()

(same for ament_uncrustify down below, which I can't put a comment in due to GitHub limitations)

Signed-off-by: Alejandro Hernandez Cordero <[email protected]>

clalancette

This looks good to me, thanks @ahcorde

Yadunund · 2025-03-20T16:02:29Z

rmw_zenoh_cpp/src/detail/graph_cache.hpp

-  // This uses a map that remembers insertion order because some parts of the client libraries
-  // expect that these are returned in the order that they were created.
-  using TopicMap = tsl::ordered_map<std::string, TopicTypeMap>;
+  // This uses a map that sort element by name because some parts of the client libraries


Nit

Suggested change

// This uses a map that sort element by name because some parts of the client libraries

// This uses a map that sorts keys alphabetically because some parts of the client libraries

ahcorde · 2025-03-20T16:10:54Z

Thank you for the review @clalancette 👍

ahcorde · 2025-03-20T16:13:11Z

https://github.com/Mergifyio backport jazzy humble

mergify · 2025-03-20T16:13:24Z

backport jazzy humble

✅ Backports have been created

#565 Switch to std::map for TopicTypeMap (backport #546) has been created for branch jazzy
#566 Switch to std::map for TopicTypeMap (backport #546) has been created for branch humble but encountered conflicts

Signed-off-by: Alejandro Hernandez Cordero <[email protected]> Co-authored-by: Yuyuan Yuan <[email protected]> (cherry picked from commit db25318)

Signed-off-by: Alejandro Hernandez Cordero <[email protected]> Co-authored-by: Yuyuan Yuan <[email protected]> (cherry picked from commit db25318) # Conflicts: # rmw_zenoh_cpp/CMakeLists.txt

Signed-off-by: Alejandro Hernandez Cordero <[email protected]> Co-authored-by: Yuyuan Yuan <[email protected]> (cherry picked from commit db25318) Co-authored-by: Alejandro Hernández Cordero <[email protected]>

* Switch to std::map for TopicTypeMap (#546) Signed-off-by: Alejandro Hernandez Cordero <[email protected]> Co-authored-by: Yuyuan Yuan <[email protected]> (cherry picked from commit db25318) Co-authored-by: Alejandro Hernández Cordero <[email protected]>

use std::map in topicMap type

73b4b17

Signed-off-by: Alejandro Hernandez Cordero <[email protected]>

ahcorde self-assigned this Mar 14, 2025

ahcorde marked this pull request as draft March 14, 2025 12:39

ahcorde mentioned this pull request Mar 14, 2025

Sort alphabetically fill_names_and_types #536

Closed

YuanYuYuan mentioned this pull request Mar 14, 2025

fix: to avoid the copy #548

Merged

fix: to avoid the copy (#548)

1e0a028

ahcorde requested a review from Yadunund March 17, 2025 16:54

Yadunund reviewed Mar 17, 2025

View reviewed changes

ahcorde marked this pull request as ready for review March 18, 2025 11:02

Yadunund requested changes Mar 18, 2025

View reviewed changes

Yadunund changed the title ~~use std::map in topicMap type~~ Switch to std::map for TopicTypeMap Mar 18, 2025

ahcorde added 2 commits March 19, 2025 22:36

Merge remote-tracking branch 'origin/rolling' into ahcorde/rolling/to…

966be32

…pic_map

review feedback

20ee1ce

Signed-off-by: Alejandro Hernandez Cordero <[email protected]>

ahcorde requested a review from Yadunund March 19, 2025 21:48

clalancette requested changes Mar 20, 2025

View reviewed changes

Removed MIT license from package.xml

973935b

Signed-off-by: Alejandro Hernandez Cordero <[email protected]>

ahcorde requested a review from clalancette March 20, 2025 14:38

clalancette requested changes Mar 20, 2025

View reviewed changes

review feedback

51fd785

Signed-off-by: Alejandro Hernandez Cordero <[email protected]>

ahcorde requested a review from clalancette March 20, 2025 14:50

clalancette approved these changes Mar 20, 2025

View reviewed changes

Yadunund reviewed Mar 20, 2025

View reviewed changes

ahcorde merged commit db25318 into rolling Mar 20, 2025
5 checks passed

ahcorde deleted the ahcorde/rolling/topic_map branch March 20, 2025 16:11

mergify bot pushed a commit that referenced this pull request Mar 20, 2025

Switch to std::map for TopicTypeMap (#546)

d43c017

Signed-off-by: Alejandro Hernandez Cordero <[email protected]> Co-authored-by: Yuyuan Yuan <[email protected]> (cherry picked from commit db25318)

mergify bot mentioned this pull request Mar 20, 2025

Switch to std::map for TopicTypeMap (backport #546) #565

Merged

mergify bot mentioned this pull request Mar 20, 2025

Switch to std::map for TopicTypeMap (backport #546) #566

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to std::map for TopicTypeMap #546

Switch to std::map for TopicTypeMap #546

ahcorde commented Mar 14, 2025

Yadunund left a comment

Yadunund left a comment •

edited

Loading

Yadunund Mar 18, 2025

clalancette commented Mar 20, 2025

clalancette left a comment

clalancette Mar 20, 2025

clalancette left a comment

Yadunund Mar 20, 2025

ahcorde commented Mar 20, 2025

ahcorde commented Mar 20, 2025

mergify bot commented Mar 20, 2025 •

edited

Loading

	run: /ros_entrypoint.sh ament_uncrustify rmw_zenoh_cpp/ --exclude rmw_zenoh_cpp/src/detail/ordered_hash.hpp rmw_zenoh_cpp/src/detail/ordered_map.hpp
	- name: cpplint
	run: /ros_entrypoint.sh ament_cpplint rmw_zenoh_cpp/ --exclude rmw_zenoh_cpp/src/detail/ordered_hash.hpp rmw_zenoh_cpp/src/detail/ordered_map.hpp

	// This uses a map that sort element by name because some parts of the client libraries
	// This uses a map that sorts keys alphabetically because some parts of the client libraries

Switch to std::map for TopicTypeMap #546

Switch to std::map for TopicTypeMap #546

Conversation

ahcorde commented Mar 14, 2025

Yadunund left a comment

Choose a reason for hiding this comment

Yadunund left a comment • edited Loading

Choose a reason for hiding this comment

Yadunund Mar 18, 2025

Choose a reason for hiding this comment

clalancette commented Mar 20, 2025

clalancette left a comment

Choose a reason for hiding this comment

clalancette Mar 20, 2025

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

Yadunund Mar 20, 2025

Choose a reason for hiding this comment

ahcorde commented Mar 20, 2025

ahcorde commented Mar 20, 2025

mergify bot commented Mar 20, 2025 • edited Loading

✅ Backports have been created

Yadunund left a comment •

edited

Loading

mergify bot commented Mar 20, 2025 •

edited

Loading