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

One NodeShared per process #152

Merged
merged 16 commits into from
Sep 4, 2020
Merged

One NodeShared per process #152

merged 16 commits into from
Sep 4, 2020

Conversation

joxoby
Copy link
Contributor

@joxoby joxoby commented Jul 25, 2020 •
edited
Loading

This PR tries to address: #137

@joxoby joxoby requested a review from caguero as a code owner July 25, 2020 02:46
caguero
caguero requested changes Jul 27, 2020
View reviewed changes
Copy link
Collaborator

@caguero caguero left a comment

Choose a reason for hiding this comment

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

Thanks @joxoby for the patch. I'll take a look at it in the next days. In the meantime, could you fix a few minor style errors?

See Bionic CI->Code check section here.

https://github.com/ignitionrobotics/ign-transport/pull/152/checks?check_run_id=912255849

@chapulina chapulina added the 🏰 citadel Ignition Citadel label Jul 27, 2020
caguero and others added 9 commits July 27, 2020 11:26
@caguero
Remove Windows warnings (gazebosim#151)
7ee4db8 
Signed-off-by: Carlos Aguero <[email protected]>
Signed-off-by: Juan Oxoby <[email protected]>
One NodeShared per process
46ebe12 
Signed-off-by: Juan Oxoby <[email protected]>
@caguero @chapulina
Remove warnings on Homebrew (gazebosim#150)
202b070 
Signed-off-by: Carlos Aguero <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>
Signed-off-by: Juan Oxoby <[email protected]>
Make NodeShared thread-safe
b0fffb7 
Signed-off-by: Juan Oxoby <[email protected]>
General fixes
ec070e6 
Signed-off-by: Juan Oxoby <[email protected]>
Fix comments
940374b 
Signed-off-by: Juan Oxoby <[email protected]>
Remove blank line
0538ae6 
Signed-off-by: Juan Oxoby <[email protected]>
code_check fixes
bc1d5f5 
Signed-off-by: Juan Oxoby <[email protected]>
Prevent cpplint from complaining because of <shared_mutex>
b9b61b7 
Signed-off-by: Juan Oxoby <[email protected]>
@joxoby joxoby requested a review from caguero July 27, 2020 18:35
Rename variables
71677b3 
Signed-off-by: Juan Oxoby <[email protected]>
@caguero
Copy link
Collaborator

caguero commented Aug 10, 2020

@osrf-jenkins run tests please

caguero
caguero requested changes Aug 10, 2020
View reviewed changes
Copy link
Collaborator

@caguero caguero left a comment

Choose a reason for hiding this comment

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

Thanks @joxoby for the patch. CI is failing on Windows. See here.

Is your approach using ::getpid() portable? In other words, does this function exist on Windows?

Make it portable
4bdb548 
Signed-off-by: Juan Oxoby <[email protected]>
@joxoby joxoby requested a review from caguero August 18, 2020 01:50
Use header windows.h
ba0feb8 
Signed-off-by: Juan Oxoby <[email protected]>
@joxoby
Copy link
Contributor Author

joxoby commented Aug 18, 2020

@caguero is test TEST(twoProcPubSub, SubThrottled) during check ignition_transport-ci-pr_any-homebrew-amd64 known to fail?

@caguero
Copy link
Collaborator

caguero commented Aug 28, 2020 •
edited
Loading

@caguero is test TEST(twoProcPubSub, SubThrottled) during check ignition_transport-ci-pr_any-homebrew-amd64 known to fail?

Yes, it's a flaky test on Homebrew.

caguero
caguero approved these changes Sep 4, 2020
View reviewed changes
Copy link
Collaborator

@caguero caguero left a comment

Choose a reason for hiding this comment

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

Looks good to me. I like the use of std::shared_mutex to optimize the most common operation.

Thanks!

@caguero caguero merged commit cd083e3 into gazebosim:ign-transport8 Sep 4, 2020
@traversaro traversaro mentioned this pull request Nov 1, 2020
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caguero caguero caguero approved these changes

Assignees

@caguero caguero

Labels
🏰 citadel Ignition Citadel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@joxoby @caguero @chapulina