-
Notifications
You must be signed in to change notification settings - Fork 402
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 #1142 add service discovery c binding #1160
Iox #1142 add service discovery c binding #1160
Conversation
4bb66e7
to
cf121cc
Compare
iceoryx_binding_c/include/iceoryx_binding_c/service_discovery.h
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #1160 +/- ##
==========================================
+ Coverage 77.78% 78.12% +0.34%
==========================================
Files 352 353 +1
Lines 14291 14408 +117
Branches 2014 2030 +16
==========================================
+ Hits 11116 11256 +140
+ Misses 2494 2460 -34
- Partials 681 692 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review
iceoryx_binding_c/include/iceoryx_binding_c/service_discovery.h
Outdated
Show resolved
Hide resolved
// the value of the array size is the result of the following formula: | ||
// sizeof(ServiceDiscovery) / 8 | ||
#if defined(__APPLE__) | ||
uint64_t do_not_touch_me[49175]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appears fairly small (if it has space for 1024 Servicedescriptions each larger than 300 bytes). I am not sure something is off here. But will likely change (to something smaller) after #1151 because than we store those in dynamic memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got the numbers via "CI Debugging". Don't forget that you have to multiply by 8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes ... it is a uint64_t. Why I do not know (it is 8 byte aligned then, machine word on 64 bit architectures). But I am not sure access is always aligned for the actual type (many architectures can compensate this). Note that placement new requires an aligned pointer, which is not generally true for the ones we pass (&do_not_touch_me
).
TLDR: I think this is broken for some types (those with alignment greater than 8) but have not looked at the details (this is nothing particular to this PR). It is broken on machines that cannot enforce alignment on their own and even then the space may be insufficient.
If we do not have alignment > 8 types then it is ok, if we add those types in the future it can subtly break on some architectures.
iceoryx_posh/include/iceoryx_posh/runtime/service_discovery.hpp
Outdated
Show resolved
Hide resolved
iceoryx_posh/include/iceoryx_posh/runtime/service_discovery.hpp
Outdated
Show resolved
Hide resolved
iceoryx_posh/include/iceoryx_posh/runtime/service_discovery.hpp
Outdated
Show resolved
Hide resolved
e54f2fb
34bd520
to
e54f2fb
Compare
Signed-off-by: Marika Lehmann <[email protected]>
Signed-off-by: Marika Lehmann <[email protected]>
Signed-off-by: Marika Lehmann <[email protected]>
Signed-off-by: Marika Lehmann <[email protected]>
Signed-off-by: Marika Lehmann <[email protected]>
…ation_info Signed-off-by: Marika Lehmann <[email protected]>
Signed-off-by: Marika Lehmann <[email protected]>
Signed-off-by: Marika Lehmann <[email protected]>
Check if Semaphore::post was successful and adapt array size of service discovery Signed-off-by: Marika Lehmann <[email protected]>
Add nullptr check for missed elements ptr, replace magic number in test, add findService function without context data, correct doxygen comments Signed-off-by: Marika Lehmann <[email protected]>
Remove nullptr check for context data, reset callback pointer in test setup Signed-off-by: Marika Lehmann <[email protected]>
…tests Signed-off-by: Marika Lehmann <[email protected]>
Signed-off-by: Marika Lehmann <[email protected]>
e54f2fb
to
73df477
Compare
There is some leak problem which the address sanitizer appears to detect. |
Signed-off-by: Marika Lehmann <[email protected]>
Pre-Review Checklist for the PR Author
iox-#123-this-is-a-branch
)iox-#123 commit text
)git commit -s
)task-list-completed
)Notes for Reviewer
This PR introduces the service discovery C binding. The examples will follow in another PR.
Checklist for the PR Reviewer
Post-review Checklist for the PR Author
References