-
Notifications
You must be signed in to change notification settings - Fork 388
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 #1036 in place creation for shared memory #1053
Iox #1036 in place creation for shared memory #1053
Conversation
635c3c5
to
c5c6c13
Compare
207ddc8
to
265ef57
Compare
Codecov Report
@@ Coverage Diff @@
## master #1053 +/- ##
==========================================
- Coverage 76.16% 76.12% -0.05%
==========================================
Files 343 344 +1
Lines 13003 13030 +27
Branches 1870 1869 -1
==========================================
+ Hits 9904 9919 +15
- Misses 2482 2495 +13
+ Partials 617 616 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
...yx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/shared_memory_object/shared_memory.hpp
Show resolved
Hide resolved
iceoryx_hoofs/source/posix_wrapper/shared_memory_object/shared_memory.cpp
Show resolved
Hide resolved
...yx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/shared_memory_object/shared_memory.hpp
Outdated
Show resolved
Hide resolved
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.
Could you please split this PR up and create a separate PR for the filesystem
feature
@elBoberido I created the pull request #1061 and a separate issue #1059 which only handles the C++17 perms enum class. |
d5bb04a
to
598fd56
Compare
598fd56
to
beb4da1
Compare
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.
Looks good. Only minor findings
...yx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/shared_memory_object/shared_memory.hpp
Outdated
Show resolved
Hide resolved
...yx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/shared_memory_object/shared_memory.hpp
Outdated
Show resolved
Hide resolved
cxx::string<SharedMemory::Name_t::capacity() + 1> addLeadingSlash(const SharedMemory::Name_t& name) noexcept | ||
{ | ||
cxx::string<SharedMemory::Name_t::capacity() + 1> nameWithLeadingSlash = "/"; | ||
nameWithLeadingSlash.append(cxx::TruncateToCapacity, name); |
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.
Just wondering if cxx::TruncateToCapacity
is necessary since the source string is smaller than the destination string. Is the optimization not yet implemented?
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.
It is always required since append is pure runtime since we do not know at compile time what kind of content is already present in the string. With assign
this is possible but not with append
.
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.
Okay, you are right. We have here more information than the compiler. This is nothing for this PR, but would it be possible to have a constexpr
cxx::string
? This question is directed more towards @FerdinandSpitzschnueffler
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.
@elBoberido To be honest, I'm not sure if it would suffice to make all the constructors constexpr
(the default c'tor already is), bit I'd like to try and will add a todo to #208
iceoryx_hoofs/source/posix_wrapper/shared_memory_object/shared_memory.cpp
Show resolved
Hide resolved
iceoryx_hoofs/source/posix_wrapper/shared_memory_object/shared_memory.cpp
Outdated
Show resolved
Hide resolved
iceoryx_hoofs/source/posix_wrapper/shared_memory_object/shared_memory.cpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Christian Eltzschig <me@elchris.org>
Signed-off-by: Christian Eltzschig <me@elchris.org>
…ing slash, shared memory builder implemented Signed-off-by: Christian Eltzschig <me@elchris.org>
…angelog update Signed-off-by: Christian Eltzschig <me@elchris.org>
…ion, remove unnecessary / from shared memory name Signed-off-by: Christian Eltzschig <me@elchris.org>
…ding slash in unlinkIfExists Signed-off-by: Christian Eltzschig <me@elchris.org>
beb4da1
to
a085d46
Compare
…s, cleanup shm file handle one failure, unlink shm when owned on failure Signed-off-by: Christian Eltzschig <me@elchris.org>
iceoryx_hoofs/source/posix_wrapper/shared_memory_object/shared_memory.cpp
Outdated
Show resolved
Hide resolved
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.
Only one minor thing. It's up to you if you want this change.
iceoryx_hoofs/source/posix_wrapper/shared_memory_object/shared_memory.cpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Christian Eltzschig <me@elchris.org>
… usage Signed-off-by: Christian Eltzschig <me@elchris.org>
Signed-off-by: Christian Eltzschig <me@elchris.org>
a1b44a8
to
eee632b
Compare
Signed-off-by: Christian Eltzschig <me@elchris.org>
…pipe Signed-off-by: Christian Eltzschig <me@elchris.org>
Pre-Review Checklist for the PR Author
iox-#123-this-is-a-branch
)iox-#123 commit text
)git commit -s
)task-list-completed
)Notes for Reviewer
This PR introduces the
SharedMemoryBuilder
for theSharedMemory
class and removes the creation pattern. Furthermore, the leading slash requirement is fixed as well which closes #439Checklist for the PR Reviewer
Post-review Checklist for the PR Author
References