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 Sample class Refactoring #871

Closed

Conversation

Indra5196
Copy link

@Indra5196 Indra5196 commented Jul 5, 2021

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

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
  • 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

@Indra5196 Indra5196 changed the title iox-#27 Sample class Refactoring iox #27 Sample class Refactoring Jul 5, 2021
@codecov
Copy link

codecov bot commented Jul 5, 2021

Codecov Report

Merging #871 (605f6db) into master (2685910) will decrease coverage by 0.02%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #871      +/-   ##
==========================================
- Coverage   75.62%   75.60%   -0.03%     
==========================================
  Files         335      337       +2     
  Lines       12219    12228       +9     
  Branches     2037     2037              
==========================================
+ Hits         9241     9245       +4     
- Misses       2194     2197       +3     
- Partials      784      786       +2     
Flag Coverage Δ
unittests 74.52% <75.00%> (-0.01%) ⬇️
unittests_timing 29.72% <0.00%> (-0.03%) ⬇️

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

Impacted Files Coverage Δ
iceoryx_posh/include/iceoryx_posh/popo/sample.hpp 66.66% <0.00%> (+9.52%) ⬆️
...ryx_posh/include/iceoryx_posh/popo/smart_chunk.hpp 57.14% <57.14%> (ø)
...include/iceoryx_posh/internal/popo/smart_chunk.inl 77.77% <77.77%> (ø)
...posh/include/iceoryx_posh/internal/popo/sample.inl 100.00% <100.00%> (+17.14%) ⬆️
iceoryx_hoofs/source/concurrent/loffli.cpp 85.29% <0.00%> (-5.89%) ⬇️
iceoryx_hoofs/source/posix_wrapper/timer.cpp 60.60% <0.00%> (-0.87%) ⬇️

@elBoberido elBoberido mentioned this pull request Jul 5, 2021
20 tasks
@elBoberido elBoberido added enhancement New feature refactoring Refactor code without adding features labels Jul 5, 2021
Copy link
Member

@elBoberido elBoberido left a comment

Choose a reason for hiding this comment

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

Please replace Closes TBD with Relates to #27 in order to link this PR to the issue.

Please also add tests for Request and Response similar to what is tested in test_popo_sample.cpp

iceoryx_posh/include/iceoryx_posh/popo/smart_chunk.hpp Outdated Show resolved Hide resolved
iceoryx_posh/include/iceoryx_posh/popo/smart_chunk.hpp Outdated Show resolved Hide resolved
iceoryx_posh/include/iceoryx_posh/internal/popo/sample.inl Outdated Show resolved Hide resolved
iceoryx_posh/include/iceoryx_posh/popo/request.hpp Outdated Show resolved Hide resolved
iceoryx_posh/include/iceoryx_posh/popo/request.hpp Outdated Show resolved Hide resolved
iceoryx_posh/include/iceoryx_posh/popo/response.hpp Outdated Show resolved Hide resolved
iceoryx_posh/include/iceoryx_posh/internal/popo/sample.inl Outdated Show resolved Hide resolved
…hanged TransmissionInterface type in case of Request and Response
/// These requests are transmittable to the iceoryx system.
///
template <typename T>
class Request : public SmartChunk<RpcInterface<Request<T>>, T, RequestHeader>
Copy link
Member

Choose a reason for hiding this comment

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

I think once you create a test for this class, this won't compile since there is no RpcInterface defined anywhere.

Also, at some places you use RcpInterface<T> which is passed to Base_t which has a RpcInterface<Request<T>>.

Could you please create some tests like in test_popo_sample.cpp but for Request and Response

Copy link
Member

Choose a reason for hiding this comment

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

oh, I just noticed that there is a rpc_interface.hpp added in #881. Please add it in this PR

/// @brief Retrieve the request header of the underlying memory chunk loaned to the request.
/// @return The request header of the underlying memory chunk.
///
RequestHeader& getRequestHeader() noexcept;
Copy link
Member

Choose a reason for hiding this comment

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

please use cxx::add_const_conditionally_t similar to what is done with the ChunkHeader to make this const in case T has the const qualifier

/// @brief Retrieve the response header of the underlying memory chunk loaned to the response.
/// @return The response header of the underlying memory chunk.
///
ResponseHeader& getResponseHeader() noexcept;
Copy link
Member

Choose a reason for hiding this comment

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

same here

@elBoberido
Copy link
Member

@Indra5196 just want to check the status of this PR. Are you working on the tests or are you waiting for some feedback from me?

@elBoberido
Copy link
Member

Closed in favor of #1081

@elBoberido elBoberido closed this Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature refactoring Refactor code without adding features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants