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

RFC: Add cista::*::cstring type #202

Merged
merged 1 commit into from
Nov 25, 2023

Conversation

khng300
Copy link
Contributor

@khng300 khng300 commented Oct 29, 2023

This new type is able to store a trailing \0 character, without compromising one byte for storage when the string is a small-string.

Storage of NUL character within data is also supported.

This is inspired by
#187 (comment).

See #187.

@khng300 khng300 changed the title Add cista::*::cstring type RFC: Add cista::*::cstring type Oct 29, 2023
@felixguendling
Copy link
Owner

Thank you for your PR! I think some tests would be useful, formatting should be fixed (using clang-format), and maybe a bit of description in the code how it works could help me to understand what's going on (just the tricky spots where the magic happens).

@khng300 khng300 force-pushed the add-cstring-rfc branch 4 times, most recently from 50e0454 to 0079634 Compare November 12, 2023 05:30
Copy link
Owner

@felixguendling felixguendling left a comment

Choose a reason for hiding this comment

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

Looks great! I guess some tests for serialization and deserialization would be nice :)

@felixguendling
Copy link
Owner

Still in draft mode. Something missing?

@khng300
Copy link
Contributor Author

khng300 commented Nov 25, 2023

I was considering if it is still worth keeping cstring_view in such case. If not, the corner case of checking null can be omitted.

@khng300 khng300 marked this pull request as ready for review November 25, 2023 06:16
@khng300 khng300 force-pushed the add-cstring-rfc branch 2 times, most recently from c07e7e7 to c8cef18 Compare November 25, 2023 06:20
@khng300
Copy link
Contributor Author

khng300 commented Nov 25, 2023

Still in draft mode. Something missing?

The latest commit pulled out the null-pointer checks for set_owning related calls().

@khng300 khng300 force-pushed the add-cstring-rfc branch 2 times, most recently from befa740 to 4caf71b Compare November 25, 2023 06:52
This new type is able to store a trailing \0 character, without
compromising one byte for storage when the string is a small-string.

Storage of NUL character within data is also supported.

This is inspired by
felixguendling#187 (comment).

See felixguendling#187.
@khng300
Copy link
Contributor Author

khng300 commented Nov 25, 2023

  • Guard against the case of nullptr with non-zero length.

@felixguendling felixguendling merged commit 79af417 into felixguendling:master Nov 25, 2023
10 checks passed
@felixguendling
Copy link
Owner

Thank you!

@khng300 khng300 deleted the add-cstring-rfc branch February 19, 2024 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants