-
Notifications
You must be signed in to change notification settings - Fork 47
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
[#210] Fix node and service name C API #405
[#210] Fix node and service name C API #405
Conversation
@@ -29,12 +29,12 @@ fn basic_node_name_test() -> Result<(), Box<dyn std::error::Error>> { | |||
assert_that!(ret_val, eq(IOX2_OK)); | |||
|
|||
let mut node_name_len = 0; | |||
let node_name_c_str = iox2_node_name_as_c_str( | |||
let node_name_c_ptr = iox2_node_name_as_str( |
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.
@elBoberido The name node_name_c_ptr
causes me to scratch my head on first read. It's because it is just a pointer to chars (i.e. not null terminated)?
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.
@elBoberido Maybe node_name_chars
?
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.
Should the function then be named iox2_node_name_as_chars
?
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.
@elBoberido Works for me
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.
I liked the function name since str
has this implicit null-termination in the C world and chars
is great for just a char array.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #405 +/- ##
==========================================
- Coverage 78.99% 78.95% -0.04%
==========================================
Files 194 194
Lines 22730 22730
==========================================
- Hits 17955 17946 -9
- Misses 4775 4784 +9 |
We reached the Cirrus CI limit. Merging anyway since the github CI is our main CI. |
Notes for Reviewer
The node and service name C-API pretent to return a null terminated C-style string. This is not the case though, since the Rust
NodeName
andServiceName
is based on a RustString
and not on an iceoryx2SemanticString
.This PR fixes the documentation and makes passing a pointer for the string length mandatory.
Pre-Review Checklist for the PR Author
SPDX-License-Identifier: Apache-2.0 OR MIT
iox2-123-introduce-posix-ipc-example
)[#123] Add posix ipc example
)task-list-completed
)Checklist for the PR Reviewer
Post-review Checklist for the PR Author
References
Relates to #210