-
Notifications
You must be signed in to change notification settings - Fork 392
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
iox-#1196 activate clang-tidy check on posix wrapper #1538
iox-#1196 activate clang-tidy check on posix wrapper #1538
Conversation
…ove test files to follow file naming guideline Signed-off-by: Christian Eltzschig <[email protected]>
Signed-off-by: Christian Eltzschig <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #1538 +/- ##
=======================================
Coverage 78.88% 78.88%
=======================================
Files 378 379 +1
Lines 14483 14489 +6
Branches 2028 2028
=======================================
+ Hits 11425 11430 +5
- Misses 2431 2432 +1
Partials 627 627
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
||
if (name[0] == '/') | ||
{ | ||
name.substr(1).and_then([&](auto& s) { channelName.append(cxx::TruncateToCapacity, s); }); |
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.
Shouldn't we at least drop a warning or information to the user that we remove the leading slash here?
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.
No not at all. In the end the ipc channel contract is to provide a valid file name (which does not allow slashes) in the IPC channel.
This still needs to be refactored somehow, since some IPC channels require slashes (message queue), some allow full paths (named pipe) and some only require a valid file name (unix domain socket).
This all has to be handled by an abstraction above those constructs which will be introduced in: #1451
Until then I would leave it as it is and then refactor it step by step especially since the NamedPipe
is only used in Windows.
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.
LGTM, left one question.
iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/shared_memory_object.hpp
Show resolved
Hide resolved
Signed-off-by: Christian Eltzschig <[email protected]>
Pre-Review Checklist for the PR Author
iox-123-this-is-a-branch
)iox-#123 commit text
)git commit -s
)task-list-completed
)./clang-tidy-diff-scans.txt
Notes for Reviewer
Checklist for the PR Reviewer
./clang-tidy-diff-scans.txt
Post-review Checklist for the PR Author
References