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

Iox #27 add untyped client and server api [stacked PR #4] #1091

Merged
merged 17 commits into from
Feb 21, 2022

Conversation

elBoberido
Copy link
Member

@elBoberido elBoberido commented Feb 10, 2022

Pre-Review Checklist for the PR Author

  1. Code follows the coding style of CONTRIBUTING.md
  2. Tests follow the best practice for testing
  3. Changelog updated in the unreleased section including API breaking changes
  4. Branch follows the naming format (iox-#123-this-is-a-branch)
  5. Commits messages are according to this guideline
    • Commit messages have the issue ID (iox-#123 commit text)
    • Commit messages are signed (git commit -s)
    • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
  6. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  7. Relevant issues are linked
  8. Add sensible notes for the reviewer
  9. All checks have passed (except task-list-completed)
  10. Assign PR to reviewer

Notes for Reviewer

This PR add the untyped API for the Client and Server. The functionality is complete.

The changelog will be updated once the feature is finished.

Test will be added in PR #1138

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Code according to our coding style and naming conventions
  • Unit tests have been written for new behavior
    • Each unit test case has a unique UUID
  • Public API changes are documented via doxygen
  • Copyright owner are updated in the changed files
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

@elBoberido elBoberido added the enhancement New feature label Feb 10, 2022
@elBoberido elBoberido self-assigned this Feb 10, 2022
@elBoberido elBoberido linked an issue Feb 10, 2022 that may be closed by this pull request
22 tasks
@elBoberido elBoberido marked this pull request as draft February 10, 2022 21:58
@elBoberido elBoberido changed the base branch from master to iox-#27-add-client-and-server-port-to-runtime-and-roudi February 10, 2022 21:59
@elBoberido elBoberido force-pushed the iox-#27-add-untyped-client-and-server-api branch from cafa071 to 23359fb Compare February 10, 2022 22:35
@elBoberido elBoberido force-pushed the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch from 5a5212b to e964cf2 Compare February 11, 2022 11:05
@elBoberido elBoberido force-pushed the iox-#27-add-untyped-client-and-server-api branch from 23359fb to f346fb0 Compare February 11, 2022 11:06
@elBoberido elBoberido force-pushed the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch from e964cf2 to 41e3fd9 Compare February 11, 2022 11:23
@elBoberido elBoberido force-pushed the iox-#27-add-untyped-client-and-server-api branch from f346fb0 to 1cbc765 Compare February 11, 2022 11:24
@elBoberido elBoberido mentioned this pull request Feb 11, 2022
20 tasks
@elBoberido elBoberido force-pushed the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch from 41e3fd9 to af415c1 Compare February 11, 2022 18:20
@elBoberido elBoberido force-pushed the iox-#27-add-untyped-client-and-server-api branch from 1cbc765 to 601edf6 Compare February 11, 2022 18:21
@elBoberido elBoberido force-pushed the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch from af415c1 to 31d1487 Compare February 14, 2022 15:20
@elBoberido elBoberido force-pushed the iox-#27-add-untyped-client-and-server-api branch from 891b47e to e74429c Compare February 14, 2022 15:21
@elBoberido elBoberido force-pushed the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch from 31d1487 to 26e1326 Compare February 15, 2022 18:56
@elBoberido elBoberido force-pushed the iox-#27-add-untyped-client-and-server-api branch from f20bad9 to c02c9f5 Compare February 15, 2022 18:58
@elBoberido elBoberido force-pushed the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch from 26e1326 to 4f7cb9c Compare February 15, 2022 19:45
@elBoberido elBoberido force-pushed the iox-#27-add-untyped-client-and-server-api branch from c02c9f5 to 9cf7047 Compare February 15, 2022 19:46
@elBoberido elBoberido changed the title [do-not-review] Iox #27 add untyped client and server api [stacked PR #4] Iox #27 add untyped client and server api [stacked PR #4] Feb 15, 2022
@elBoberido elBoberido force-pushed the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch from 4f7cb9c to 80fecba Compare February 16, 2022 17:52
@elBoberido elBoberido force-pushed the iox-#27-add-untyped-client-and-server-api branch from 9cf7047 to 587970e Compare February 16, 2022 17:53
@budrus budrus self-requested a review February 17, 2022 09:22
Copy link
Contributor

@elfenpiff elfenpiff left a comment

Choose a reason for hiding this comment

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

Could you please add nullptr tests for the methods which have a pointer argument and where there is a potential segfault hidden.
For those methods which also return a pointer I would return a nullptr when they are called with a nullptr then we would be at least consistent with the remaining code base.

/// @brief Get the service description of the client.
/// @return The service description.
///
capro::ServiceDescription getServiceDescription() const noexcept;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to return here const capro::ServiceDescription & . After creation it should not change anymore or do we run then into consistency issues with the other ports?

Copy link
Member Author

Choose a reason for hiding this comment

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

I also think it is save to return a reference. Shall I also change it for the publisher and subscriber?

iceoryx_posh/source/popo/rpc_header.cpp Outdated Show resolved Hide resolved
iceoryx_posh/source/popo/rpc_header.cpp Outdated Show resolved Hide resolved
iceoryx_posh/source/popo/rpc_header.cpp Outdated Show resolved Hide resolved
iceoryx_posh/source/popo/rpc_header.cpp Outdated Show resolved Hide resolved
budrus
budrus previously approved these changes Feb 17, 2022
Copy link
Contributor

@budrus budrus left a comment

Choose a reason for hiding this comment

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

Nice reading

@elBoberido elBoberido force-pushed the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch from 80fecba to b6dea2e Compare February 17, 2022 15:03
@elBoberido elBoberido force-pushed the iox-#27-add-untyped-client-and-server-api branch from 587970e to f1198bd Compare February 17, 2022 15:04
@elBoberido elBoberido force-pushed the iox-#27-add-client-and-server-port-to-runtime-and-roudi branch from b6dea2e to cdafb14 Compare February 18, 2022 16:31
@elBoberido elBoberido force-pushed the iox-#27-add-untyped-client-and-server-api branch from f1198bd to 167c233 Compare February 18, 2022 16:42
@elBoberido elBoberido marked this pull request as ready for review February 18, 2022 21:38
@elBoberido elBoberido force-pushed the iox-#27-add-untyped-client-and-server-api branch from 7947926 to b73bbcc Compare February 21, 2022 17:39
@elBoberido elBoberido force-pushed the iox-#27-add-untyped-client-and-server-api branch from b73bbcc to 432dd56 Compare February 21, 2022 17:47
elfenpiff
elfenpiff previously approved these changes Feb 21, 2022
@codecov
Copy link

codecov bot commented Feb 21, 2022

Codecov Report

Merging #1091 (af45306) into master (8f7fc7f) will decrease coverage by 0.07%.
The diff coverage is 26.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1091      +/-   ##
==========================================
- Coverage   76.29%   76.21%   -0.08%     
==========================================
  Files         346      346              
  Lines       13585    13603      +18     
  Branches     1935     1939       +4     
==========================================
+ Hits        10364    10367       +3     
- Misses       2589     2607      +18     
+ Partials      632      629       -3     
Flag Coverage Δ
unittests 75.46% <26.92%> (-0.09%) ⬇️
unittests_timing 12.00% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ude/iceoryx_posh/internal/popo/ports/base_port.hpp 100.00% <ø> (ø)
...oryx_posh/internal/popo/ports/client_port_user.hpp 100.00% <ø> (ø)
...oryx_posh/internal/popo/ports/server_port_user.hpp 100.00% <ø> (ø)
iceoryx_posh/source/popo/rpc_header.cpp 75.51% <0.00%> (-24.49%) ⬇️
...ceoryx_posh/source/popo/ports/client_port_user.cpp 92.30% <40.00%> (-4.47%) ⬇️
...ceoryx_posh/source/popo/ports/server_port_user.cpp 96.38% <50.00%> (-3.62%) ⬇️
iceoryx_posh/source/popo/ports/base_port.cpp 88.46% <66.66%> (ø)
iceoryx_hoofs/source/posix_wrapper/timer.cpp 63.48% <0.00%> (+0.41%) ⬆️
...nternal/relocatable_pointer/pointer_repository.inl 88.63% <0.00%> (+2.27%) ⬆️
iceoryx_hoofs/source/concurrent/loffli.cpp 91.42% <0.00%> (+2.85%) ⬆️

budrus
budrus previously approved these changes Feb 21, 2022
@elBoberido elBoberido dismissed stale reviews from budrus and elfenpiff via af45306 February 21, 2022 19:22
@elBoberido elBoberido merged commit 9576821 into master Feb 21, 2022
@elBoberido elBoberido deleted the iox-#27-add-untyped-client-and-server-api branch February 21, 2022 20:05
@elBoberido elBoberido restored the iox-#27-add-untyped-client-and-server-api branch February 21, 2022 20:17
@elBoberido elBoberido deleted the iox-#27-add-untyped-client-and-server-api branch February 21, 2022 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request/Response communication with iceoryx
3 participants