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.

Sign up for GitHub

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

Jump to bottom

C++ SHM #49

Merged
merged 9 commits into from
Dec 27, 2024
Merged

C++ SHM #49

merged 9 commits into from
Dec 27, 2024

Conversation

yellowhatter
Copy link

No description provided.

@yellowhatter yellowhatter self-assigned this Dec 25, 2024
@yellowhatter yellowhatter marked this pull request as ready for review December 25, 2024 13:07
YuanYuYuan
YuanYuYuan requested changes Dec 26, 2024
View reviewed changes
Copy link
Collaborator

@YuanYuYuan YuanYuYuan left a comment

Choose a reason for hiding this comment

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

In addition to the comments I left, I'd like to discuss on some points.

  1. We allow the users to build rmw_zenoh_cpp without SHM feature. What if a user enables the SHM feature in the config that is the default behavior and builds the rmw_zenoh_cpp without SHM support? It seems not to report any error.
  2. Can you change the base branch to the rolling? The upstream has been ported to zenoh-cpp.

zenoh_cpp_vendor/CMakeLists.txt Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/shm_context.hpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/shm_context.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/shm_context.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/rmw_context_impl_s.cpp Show resolved Hide resolved
@yellowhatter yellowhatter changed the base branch from ahcorde/dev/1.0.0-cpp to rolling December 26, 2024 10:31
@yellowhatter
C++ SHM
ce1216f 
Conflicts:
	rmw_zenoh_cpp/src/detail/rmw_publisher_data.cpp
@yellowhatter
fix default shm msg size threshold
0e9ace8
@yellowhatter
code format fix
b3d0a47
evshary
evshary approved these changes Dec 27, 2024
View reviewed changes
Copy link

@evshary evshary left a comment

Choose a reason for hiding this comment

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

LGTM, but since we have RMW_ZENOH_BUILD_WITH_SHARED_MEMORY to choose whether we want to build with shared memory, it might be good to test it with CI.

rmw_zenoh_cpp/src/detail/shm_context.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/shm_context.hpp Outdated Show resolved Hide resolved
@yellowhatter yellowhatter mentioned this pull request Dec 27, 2024
Open
@yellowhatter
Copy link
Author

LGTM, but since we have RMW_ZENOH_BUILD_WITH_SHARED_MEMORY to choose whether we want to build with shared memory, it might be good to test it with CI.

Right: #50

@yellowhatter
Copy link
Author

In addition to the comments I left, I'd like to discuss on some points.

  1. We allow the users to build rmw_zenoh_cpp without SHM feature. What if a user enables the SHM feature in the config that is the default behavior and builds the rmw_zenoh_cpp without SHM support? It seems not to report any error.
  2. Can you change the base branch to the rolling? The upstream has been ported to zenoh-cpp.

  1. done

  2. Well, this question is more related to Zenoh-Rust implementation rather than RMW. If there is no SHM feature enabled, the SHM section is just ignored. Attempts to have some warnings for combination of SHM-Config + Non-SHM build will just complexify the setup for nothing.

@yellowhatter yellowhatter merged commit b80d583 into rolling Dec 27, 2024
4 of 5 checks passed
yellowhatter added a commit that referenced this pull request Dec 27, 2024
@yellowhatter
Revert "C++ SHM (#49)"
39bf98b 
This reverts commit b80d583.
@yellowhatter yellowhatter mentioned this pull request Dec 27, 2024
Merged
yellowhatter added a commit that referenced this pull request Dec 27, 2024
@yellowhatter
Revert "C++ SHM (#49)" (#51)
9fd4ebe 
This reverts commit b80d583.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evshary evshary evshary approved these changes

@YuanYuYuan YuanYuYuan Awaiting requested review from YuanYuYuan

Assignees

@yellowhatter yellowhatter

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yellowhatter @evshary @YuanYuYuan