iox-#1431: Add unsafe_raw_access to iox::string

[lucabart97]

Implement a method that provides direct access to the string's underlying pointer. This allows data to be written directly to the string, eliminating the need for copies. The string's length is checked on each call to ensure the null terminator is in the correct position.

Pre-Review Checklist for the PR Author

1. Add a second reviewer for complex new features or larger refactorings
2. Code follows the coding style of CONTRIBUTING.md
3. Tests follow the best practice for testing
4. Changelog updated in the unreleased section including API breaking changes
5. Branch follows the naming format (iox-123-this-is-a-branch)
6. Commits messages are according to this guideline
   • Commit messages have the issue ID (iox-#123 commit text)
   • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
7. Update the PR title
   • Follow the same conventions as for commit messages
   • Link to the relevant issue
8. Relevant issues are linked
9. Add sensible notes for the reviewer
10. All checks have passed (except task-list-completed)
11. All touched (C/C++) source code files from iceoryx_hoofs are added to ./clang-tidy-diff-scans.txt
12. Assign PR to reviewer

Notes for Reviewer

Checklist for the PR Reviewer

• Commits are properly organized and messages are according to the guideline
• Code according to our coding style and naming conventions
• Unit tests have been written for new behavior
• Public API changes are documented via doxygen
• Copyright owner are updated in the changed files
• All touched (C/C++) source code files from iceoryx_hoofs have been added to ./clang-tidy-diff-scans.txt
• PR title describes the changes

Post-review Checklist for the PR Author

1. All open points are addressed and tracked via issues

References

• Refer Convenience functions for cxx::string #1431

[codecov]

Codecov Report

Merging #2093 (79cd989) into master (51a018a) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2093      +/-   ##
==========================================
+ Coverage   80.28%   80.29%   +0.01%     
==========================================
  Files         417      417              
  Lines       16121    16128       +7     
  Branches     2250     2250              
==========================================
+ Hits        12942    12950       +8     
  Misses       2384     2384              
+ Partials      795      794       -1     

Flag	Coverage Δ
unittests	80.08% <100.00%> (+0.01%)	⬆️
unittests_timing	15.29% <0.00%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...ryx_hoofs/vocabulary/include/iox/detail/string.inl	96.13% <100.00%> (+0.10%)	⬆️
iceoryx_hoofs/vocabulary/include/iox/string.hpp	100.00% <ø> (ø)

... and 1 file with indirect coverage changes

[elBoberido]

For traceability reasons it might be better to have this PR to be part of #1431 which is more related to strings than #2040. Maybe we can make #2040 kind of a meta issue and have more specific issues for the tasks which need to be done. There will be a few preparatory PRs like this until everything is in place to finally get rid of the runtime allocations and not relying on small object optimizations to not have them.

[lucabart97]

For traceability reasons it might be better to have this PR to be part of #1431 which is more related to strings than #2040. Maybe we can make #2040 kind of a meta issue and have more specific issues for the tasks which need to be done. There will be a few preparatory PRs like this until everything is in place to finally get rid of the runtime allocations and not relying on small object optimizations to not have them.

@elBoberido if you prefer I can force-push the commit, but the branch style name will be the same, otherwise, I can close this PR and reopen another with the correct branch identification (using 1431 instead of 2040).

Yes, maybe is better to create a sort of tasklist for the 2040

[elBoberido]

@elBoberido if you prefer I can force-push the commit, but the branch style name will be the same, otherwise, I can close this PR and reopen another with the correct branch identification (using 1431 instead of 2040).

Yes, maybe is better to create a sort of tasklist for the 2040

I would be fine with a force push and keeping the branch name with 2040 for this PR. The most important part for traceability is what git blame shows on the respective line and to easily find the corresponding issue where the line was changed. The branch name is not that important and we must not be more catholic than the pope :)

[elBoberido]

Btw, the clang-tidy check fails. You can run it locally with tools/scripts/clang_tidy_check.sh scan_list .clang-tidy-diff-scans.txt "path/to/file1.cpp path/to/file2.cpp ..."

[lucabart97]

Btw, the clang-tidy check fails. You can run it locally with tools/scripts/clang_tidy_check.sh scan_list .clang-tidy-diff-scans.txt "path/to/file1.cpp path/to/file2.cpp ..."

I see, the strcpy should be fine in tests, so I'll add //NOLINT.. if you agree. Instead, about the other "error", what do you suggest?

adjacent parameters of 'operator()' of similar type ('const uint64_t') are easily swapped by mistake
[bugprone-easily-swappable-parameters,-warnings-as-errors]
    this->testSubject.unsafe_raw_access([this](char* str, const uint64_t size, const uint64_t capacity) {
                                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[elBoberido]

I see, the strcpy should be fine in tests, so I'll add //NOLINT.. if you agree. Instead, about the other "error", what do you suggest?

adjacent parameters of 'operator()' of similar type ('const uint64_t') are easily swapped by mistake
[bugprone-easily-swappable-parameters,-warnings-as-errors]
    this->testSubject.unsafe_raw_access([this](char* str, const uint64_t size, const uint64_t capacity) {
                                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

NOLINT wouldn't be a problem in this case. I think using strlcpy is not available on windows so that would require additional work in the platform layer. If you are interested you could try to add it but NOLINT is fine for the tests.

I might end up making strlcpy available on all platforms later on anyway since we are using strncpy in some places where strlcpy would be more safe.

For the other issue, in theory we decided to deactivated the rule for the time being but actually forgot to actually do it. I think the reason we forgot about it was that it did not show up in the production files we scan. If auto* str, const auto size, const auto capacity is used it also does not show up. This would be a workaround but I tend to think of a solution with a struct with two member. We could then re-use that struct also for the complementary function in vector later on. chatGPT suggest BufferInfo as name :)

It could look like this and could live in iceoryx_hoofs/buffer/

struct BufferInfo {
    uint64_t used_size{0};
    uint64_t total_size{0};
};

It would be a bit different than what we have now since it supplies the whole buffer size, including the space for the null-termination, but this would make it easier to work with functions like strlcpy or strlcat which automatically adds the null-termination.

What do you think?

[lucabart97]

@elBoberido

If possible, I suggest changing the clang-tidy rules, almost to set at 3 the bugprone-easily-swappable-parameters.

I think this workaround increases the code complexity because when I see the method prototype, I can't understand all parameters, I need to find and read the BufferInfo struct.

[elBoberido]

@lucabart97 if it is only for convenience I would take the more safe approach. Our long term plan is to use specific types as much as possible to elevate small oversights to compile time failures.

With autocompletion from the clang LSP it also just shows const iox::function<void(char*, const uint64_t, const uint64_t)>& func. A BufferInfo instead of the two uint64_t would give the developer some hints on what to expect.

The reason you understand the current signature is because you just wrote it and it is fresh in your mind but these things get blurry and in a few months it is not anymore as clear as now. User which are new to the API have to look at the documentation anyway or their IDE will tell them the meaning of the members of the struct.

Btw, have you already seen comments like this https://www.reddit.com/media?url=https%3A%2F%2Fi.redd.it%2Fhwqj7yx9vm211.jpg? Only slightly related but when writing about fading memories I remembered that I wrote a similar comment to something which was fully clear when I wrote the code :)

[lucabart97]

@elBoberido got it

I add BufferInfo and two string access functions:

• unsafe_raw_access
• unsafe_auto_raw_access

Tell me your feedback :)

[elBoberido]

Almost there. This can soon be merged :)

[elBoberido]

Oh, and there is another formalism. If there wouldn't be other changes then it could be left as is but since you have to commit anyway could you capitalize the commit message to iox-#1431 Add ...?

[lucabart97]

@elBoberido
Sorry for the copyright, I just copy-pasted from another file without reading it 😅

I got your comments and I have updated the code/commit message!

[elBoberido]

Almost there. Just two additional tests.

[elBoberido]

Great. Thanks for your patience :)

Once the CI has run this can be merged

[elBoberido]

Oh, just notice that there is an issue on the CI. It seems the new tests have an unused this capture in the lambda

[lucabart97]

@elBoberido
fixed!

[elBoberido]

Oh no, now Windows complains. You can easily find the error by searching for error C

[lucabart97]

@elBoberido
I understood the error with STRINGCAP that was not captured by lambda, but do you understand the other errors?

[elBoberido]

@lucabart97 I'm not quite sure. If it isn't an aftereffect on the first error it might be a missing const on the second lambda parameter on line 685

[elBoberido]

Or maybe there is a weird type conversion for STRINGCAP happening?

[lucabart97]

Ok, on mac-os the capture of STRINGCAPgives us an error. I use directly the &.

Let's see if Windows passes or not, maybe can be a weird type conversion for STRINGCAP

[elBoberido]

Welcome to the hell known as Windows support 😅

It seems clang based toolchains consider the capturing of STRINGCAP as superfluous. You can just capture all by reference, i.e. a singe &. Alternatively strlen or the data supplied by buffer info should also work.

[lucabart97]

I hope it will be fine!

(Windows 👎)

[elBoberido]

I've a good feeling now :)