-
Notifications
You must be signed in to change notification settings - Fork 40
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
[#491] Ensure enum string literals are null terminated #538
base: main
Are you sure you want to change the base?
[#491] Ensure enum string literals are null terminated #538
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #538 +/- ##
==========================================
- Coverage 79.13% 79.10% -0.04%
==========================================
Files 202 202
Lines 24815 24819 +4
==========================================
- Hits 19638 19632 -6
- Misses 5177 5187 +10
|
@@ -171,6 +175,7 @@ pub fn string_literal_derive(input: TokenStream) -> TokenStream { | |||
c => c.to_ascii_lowercase(), | |||
}) | |||
.collect::<String>(); | |||
enum_string_literal.push('\0'); |
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.
Oh I also run into this issue.
/// | ||
/// let v2 = MyEnum::VariantTwo; | ||
/// assert_eq!(v2.as_str_literal(), "variant two"); | ||
/// assert_eq!(v2.as_str_literal(), "variant two\0"); |
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.
In rust strings are never null terminated it is only typical for ffi bindings. Should we move string_literal_derive
into iceoryx2-ffi
since it is only used there?
@@ -118,10 +118,10 @@ pub fn placement_default_derive(input: TokenStream) -> TokenStream { | |||
/// } | |||
/// | |||
/// let v1 = MyEnum::VariantOne; | |||
/// assert_eq!(v1.as_str_literal(), "custom variant one"); | |||
/// assert_eq!(v1.as_str_literal(), "custom variant one\0"); |
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.
/// assert_eq!(v1.as_str_literal(), "custom variant one\0"); | |
/// assert_eq!(v1.as_str_literal(), c"custom variant one"); |
Can the same be made in the macro? Something like c#enum_string_literal
Notes for Reviewer
Fix bug where string literals were not null terminated.
Unrelated: Also fix typo in existing
assert_that
message.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
)Changelog updated in the unreleased section including API breaking changestask-list-completed
)Checklist for the PR Reviewer
Post-review Checklist for the PR Author
References
Closes #491