Iox #14 custom header for untyped C++ and C API #649

elBoberido · 2021-03-29T08:57:32Z

Pre-Review Checklist for the PR Author

Notes for Reviewer

This PR adds the custom header support to the C++ an C API and refactors the c++2c enum translation tests in order to discover missing enum values on the C side more easily.

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

All open points are addressed and tracked via issues

References

Partly closes Reserved chunkinfo user payload header #14

iceoryx_binding_c/include/iceoryx_binding_c/publisher.h

iceoryx_posh/include/iceoryx_posh/popo/untyped_publisher.hpp

codecov · 2021-03-29T09:27:19Z

Codecov Report

Merging #649 (51102fc) into master (948d02a) will decrease coverage by 0.01%.
The diff coverage is 73.17%.

@@            Coverage Diff             @@
##           master     #649      +/-   ##
==========================================
- Coverage   73.90%   73.89%   -0.02%     
==========================================
  Files         318      318              
  Lines       11077    11106      +29     
  Branches     1950     1955       +5     
==========================================
+ Hits         8187     8207      +20     
- Misses       2127     2130       +3     
- Partials      763      769       +6

Flag	Coverage Δ
unittests	`73.85% <73.17%> (-0.02%)`	⬇️
unittests_timing	`31.55% <0.00%> (-0.10%)`	⬇️

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

Impacted Files	Coverage Δ
...ude/iceoryx_binding_c/internal/cpp2c_publisher.hpp	`100.00% <ø> (ø)`
...osh/internal/popo/building_blocks/chunk_sender.hpp	`100.00% <ø> (ø)`
...sh/include/iceoryx_posh/popo/untyped_publisher.hpp	`100.00% <ø> (ø)`
iceoryx_binding_c/source/c_listener.cpp	`73.07% <40.00%> (+3.51%)`	⬆️
iceoryx_binding_c/source/c_publisher.cpp	`60.00% <57.14%> (+0.67%)`	⬆️
iceoryx_binding_c/source/c_subscriber.cpp	`61.40% <66.66%> (+2.14%)`	⬆️
iceoryx_binding_c/source/c_wait_set.cpp	`81.57% <66.66%> (+1.57%)`	⬆️
iceoryx_binding_c/source/c_user_trigger.cpp	`100.00% <100.00%> (ø)`
...ceoryx_binding_c/source/cpp2c_enum_translation.cpp	`100.00% <100.00%> (ø)`
...osh/internal/popo/building_blocks/chunk_sender.inl	`96.77% <100.00%> (+3.22%)`	⬆️
... and 6 more

iceoryx_binding_c/include/iceoryx_binding_c/publisher.h

iceoryx_binding_c/source/c_publisher.cpp

iceoryx_binding_c/source/cpp2c_enum_translation.cpp

iceoryx_binding_c/test/moduletests/test_cpp2c_enum_translation.cpp

iceoryx_binding_c/test/moduletests/test_publisher.cpp

mossmaurice · 2021-03-29T10:20:35Z

iceoryx_posh/include/iceoryx_posh/internal/popo/untyped_publisher.inl

-template <typename base_publisher_t>
-inline UntypedPublisherImpl<base_publisher_t>::UntypedPublisherImpl(const capro::ServiceDescription& service,
-                                                                    const PublisherOptions& publisherOptions)
+template <typename H, typename base_publisher_t>


HeaderType or Header_t? I think we're really inconsistent with naming template params, makes reading harder 😞

It's just H to be consistent with the typed API. There was already a T template parameter and then I added the H one

Please change the template param in the typed API, too. Same goes for the base_publisher_t. I think in iceoryx most of the time we use FooType.

I can change base_publisher_t (in a separate PR together with the typed API) but would keep the H.

I vote for short dummy variables in the inl, they do not have to be the same as in the hpp (readability). In fact, if they are not used even typename is enough. We need a convention for those _t suffixes but IMO they should not be used in template parameters.

iceoryx_posh/include/iceoryx_posh/popo/untyped_publisher.hpp

MatthiasKillat · 2021-03-29T11:52:01Z

doc/design/chunk_header.md

@@ -273,7 +273,8 @@ class MyChunkHeaderHook : public ChunkHeaderHook
 auto customHeaderHook = MyChunkHeaderHook<MyHeader>();
 auto pub = iox::popo::Publisher<MyPayload>(serviceDescription, customHeaderHook);
 ```
-        - it has to be decided if it's a good idea to give the user easy write access to the `ChunkHeader` or just to the `CustomHeader`
+        - alternatively, instead of the ChunkHeaderHook class, the publisher could have a method `registerDeliveryHook(std::function<void(ChunkHeader&)>)`
+        - allow the user only read access to the `ChunkHeader` and write access to the `CustomHeader`


We need to think about the access options for the header, true. std::function cannot be used in the long run, however. Maybe cxx::function will be sufficient but it is a bit problematic for unrestricted user callbacks due to its size bound.

an option would be plain C function pointer but then we would make it harder to use. I also thought about cxx::function, which would replace the std::function when it's ready. The API with the std::function would be marked as deprecated.

iceoryx_binding_c/include/iceoryx_binding_c/internal/cpp2c_publisher.hpp

iceoryx_binding_c/include/iceoryx_binding_c/publisher.h

iceoryx_binding_c/source/c_publisher.cpp

MatthiasKillat · 2021-03-29T12:16:27Z

iceoryx_posh/include/iceoryx_posh/internal/popo/untyped_publisher.inl

-template <typename base_publisher_t>
-inline UntypedPublisherImpl<base_publisher_t>::UntypedPublisherImpl(const capro::ServiceDescription& service,
-                                                                    const PublisherOptions& publisherOptions)
+template <typename H, typename base_publisher_t>


I vote for short dummy variables in the inl, they do not have to be the same as in the hpp (readability). In fact, if they are not used even typename is enough. We need a convention for those _t suffixes but IMO they should not be used in template parameters.

iceoryx_posh/include/iceoryx_posh/popo/untyped_publisher.hpp

Signed-off-by: Mathias Kraus <[email protected]>

…der-for-untyped-API Iox eclipse-iceoryx#14 custom header for untyped C++ and C API

elBoberido requested review from mossmaurice, elfenpiff and FerdinandSpitzschnueffler March 29, 2021 08:57

elBoberido commented Mar 29, 2021

View reviewed changes

iceoryx_binding_c/include/iceoryx_binding_c/publisher.h Outdated Show resolved Hide resolved

iceoryx_posh/include/iceoryx_posh/popo/untyped_publisher.hpp Show resolved Hide resolved

iceoryx_posh/include/iceoryx_posh/popo/untyped_publisher.hpp Show resolved Hide resolved

mossmaurice reviewed Mar 29, 2021

View reviewed changes

MatthiasKillat reviewed Mar 29, 2021

View reviewed changes

elBoberido added 6 commits March 29, 2021 20:40

iox-#14 add custom header to untyped C++ API

3b499e1

Signed-off-by: Mathias Kraus <[email protected]>

iox-#14 add custom header to untyped C API

75e06de

Signed-off-by: Mathias Kraus <[email protected]>

iox-#14 update ChunkHeader design document

11df832

Signed-off-by: Mathias Kraus <[email protected]>

iox-#14 refactor tests for cpp2c enum translation

0e79554

Signed-off-by: Mathias Kraus <[email protected]>

iox-#14 use constexpr instead of magic number and fix type

913ae45

Signed-off-by: Mathias Kraus <[email protected]>

iox-#14 move custom header settings to publisher struct on C API

67716d3

Signed-off-by: Mathias Kraus <[email protected]>

elBoberido force-pushed the iox-#14-custom-header-for-untyped-API branch from 2c0371e to 67716d3 Compare March 29, 2021 18:45

elBoberido added 3 commits March 29, 2021 21:05

iox-#14 add iox_pub_loan_aligned_chunk to C API

e59e1bf

Signed-off-by: Mathias Kraus <[email protected]>

iox-#14 readd some tests for the publisher C API

71a1d48

Signed-off-by: Mathias Kraus <[email protected]>

iox-#14 make error enum name more specific

a7205a6

Signed-off-by: Mathias Kraus <[email protected]>

elBoberido requested review from mossmaurice and MatthiasKillat March 29, 2021 19:26

iox-#14 add test for nullptr checks in C API

51102fc

Signed-off-by: Mathias Kraus <[email protected]>

MatthiasKillat approved these changes Mar 30, 2021

View reviewed changes

FerdinandSpitzschnueffler approved these changes Mar 30, 2021

View reviewed changes

elBoberido merged commit 45a3f41 into eclipse-iceoryx:master Mar 30, 2021

elBoberido deleted the iox-#14-custom-header-for-untyped-API branch March 30, 2021 09:52

marthtz pushed a commit to boschglobal/iceoryx that referenced this pull request May 12, 2021

Merge pull request eclipse-iceoryx#649 from ApexAI/iox-#14-custom-hea…

c6f0173

…der-for-untyped-API Iox eclipse-iceoryx#14 custom header for untyped C++ and C API

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iox #14 custom header for untyped C++ and C API #649

Iox #14 custom header for untyped C++ and C API #649

elBoberido commented Mar 29, 2021 •

edited by FerdinandSpitzschnueffler

Loading

codecov bot commented Mar 29, 2021 •

edited

Loading

mossmaurice Mar 29, 2021

elBoberido Mar 29, 2021

mossmaurice Mar 29, 2021

elBoberido Mar 29, 2021

MatthiasKillat Mar 29, 2021

MatthiasKillat Mar 29, 2021

elBoberido Mar 29, 2021

MatthiasKillat Mar 29, 2021

Iox #14 custom header for untyped C++ and C API #649

Iox #14 custom header for untyped C++ and C API #649

Conversation

elBoberido commented Mar 29, 2021 • edited by FerdinandSpitzschnueffler Loading

Pre-Review Checklist for the PR Author

Notes for Reviewer

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

References

codecov bot commented Mar 29, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elBoberido commented Mar 29, 2021 •

edited by FerdinandSpitzschnueffler

Loading

codecov bot commented Mar 29, 2021 •

edited

Loading