-
Notifications
You must be signed in to change notification settings - Fork 1
Add null_safe_string_view(const char*, std::size_t) #41
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
Conversation
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.
Pull request overview
This PR adds a new overload of null_safe_string_view that accepts a length parameter, enabling construction of string_views with a specified size while protecting against null pointers. Unlike safe_string_view, which terminates at null bytes, this new function preserves embedded null bytes within the specified length.
Key Changes
- Added
null_safe_string_view(const char*, std::size_t)overload that returns an empty string_view for null pointers and constructs a string_view with the specified length otherwise - Comprehensive test coverage for the new function including null pointers, zero lengths, and embedded null bytes
- Updated changelog to document the new function
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| include/gul17/string_util.h | Added function declaration and documentation for the new overload; modified parameter documentation for existing overload |
| src/string_util.cc | Implemented the new null_safe_string_view(const char*, std::size_t) function |
| tests/test_string_util.cc | Added comprehensive test cases covering null pointers, zero/non-zero lengths, and embedded null bytes |
| data/doxygen.h | Added changelog entry for the new function in the UNRELEASED section |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a3e4a9a to
12e7c15
Compare
[why] There are use cases for constructing a string_view with a specified size that is still protected against null pointers. This is different from safe_string_view(const char*, std::size_t) which terminates the string at a null byte. Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>
12e7c15 to
f7fd629
Compare
|
Ugg, this 'Pull Request Overview' really puts me off. I prefer a human generated explanation/overview, and this is just babble. If you absolutely need an LLM "bug finder", maybe we/you can at least re remove the blah as that increases the mental load any PR anyhow has. To the topic... a) A wonder if we can not somehow unify For me, having 4 functions for "the same" is too much. Even 3 is too much and I must have overlooked it. b) I imagine that sometimes you want to do something like this (feature idea): auto& setting_a = obj.settings.a;
std::cout << null_save_string_view(setting_a.charptr, setting_a.length, "Option A unset");The current documentation is already wrong:
Doxygen docu has copy&paste error? Hmm, I wanted to compare the existing
And that null-byte give the Hmm, still trying to come up with a feature table
So this looks like a reasonable extension/completion. But I believe we should have only 2 functions and not 4, and that the 'preserves Hmm. The existing 3 functions are ... functions. Being in this library it means they can not be optimized away, I assume? Maybe they should be templates instead. That is what I believed they were. Hmm:
Uggg: It seems the whole feature and naming is more complicated than one would expect. Maybe I'm wrong with this, but for me this feels like a thicket. Edit: I guess by b) is too much (at least right now) |
I know that naming is complicated, but... let me just point out that the functions that are now called Also, why is "the current documentation [...] already wrong"? I do not see that. The "null_safe_" functions only protect against null pointers, the "safe_" functions also terminate the string early at a null byte. In that regard, the new We can discuss if we should inline these functions, but IMO that should be a different MR because we would also touch the existing functions. |
I also mentioned that?
The examples?
I just suggested putting this kind of explanation into the docs. What I guess the change is ok, but I would like to see the documentation fixed (Soeren would fix them in an additional PR, but I would fix them en passant). And maybe an expansion in the documentation similar to what you gave above. |
|
I have fixed the Doxygen examples for two of the functions and tried to make the "first-line" description of each function as explicit as possible. Have another look – it really is an unpleasant thicket of functions now. |
[why] It is a jungle. The safe_string*() functions respect C-string style null termination, the null_safe_string*() functions only do that if they do not have a length parameter. Proposed-by: Fini Jastrow <ulf.fini.jastrow@desy.de> Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>
Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>
5168a28 to
b7e28be
Compare
|
Force-pushed with a whitespace change. |
Maybe we should try a more organized approach and clean this up. Next year. If time permits 🤣 |



There are use cases for constructing a
string_viewwith a specified size in a way that is protected against null pointers. This is notably different from the existingsafe_string_view(const char*, std::size_t)which terminates the string at a null byte.