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

Iox #1036 in place creation for shared memory #1053

Conversation

elfenpiff
Copy link
Contributor

@elfenpiff elfenpiff commented Jan 28, 2022

Pre-Review Checklist for the PR Author

  1. Code follows the coding style of CONTRIBUTING.md
  2. Tests follow the best practice for testing
  3. Changelog updated in the unreleased section including API breaking changes
  4. Branch follows the naming format (iox-#123-this-is-a-branch)
  5. Commits messages are according to this guideline
    • Commit messages have the issue ID (iox-#123 commit text)
    • Commit messages are signed (git commit -s)
    • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
  6. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  7. Relevant issues are linked
  8. Add sensible notes for the reviewer
  9. All checks have passed (except task-list-completed)
  10. Assign PR to reviewer

Notes for Reviewer

This PR introduces the SharedMemoryBuilder for the SharedMemory class and removes the creation pattern. Furthermore, the leading slash requirement is fixed as well which closes #439

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
    • Each unit test case has a unique UUID
  • Public API changes are documented via doxygen
  • Copyright owner are updated in the changed files
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

@elfenpiff elfenpiff linked an issue Jan 28, 2022 that may be closed by this pull request
12 tasks
@elfenpiff elfenpiff force-pushed the iox-#1036-in-place-creation-for-shared-memory branch from 635c3c5 to c5c6c13 Compare January 28, 2022 17:32
@elfenpiff elfenpiff self-assigned this Jan 28, 2022
@elfenpiff elfenpiff force-pushed the iox-#1036-in-place-creation-for-shared-memory branch from 207ddc8 to 265ef57 Compare January 28, 2022 18:40
@codecov
Copy link

codecov bot commented Jan 28, 2022

Codecov Report

Merging #1053 (cae20a7) into master (27b45e7) will decrease coverage by 0.04%.
The diff coverage is 80.53%.

Impacted file tree graph

@@            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     
Flag Coverage Δ
unittests 75.34% <80.53%> (-0.06%) ⬇️
unittests_timing 12.52% <0.00%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
...eoryx_hoofs/include/iceoryx_hoofs/cxx/helplets.hpp 97.29% <ø> (ø)
iceoryx_hoofs/source/posix_wrapper/named_pipe.cpp 57.67% <50.00%> (-0.88%) ⬇️
...six_wrapper/shared_memory_object/shared_memory.cpp 64.73% <77.01%> (-4.30%) ⬇️
...fs/internal/posix_wrapper/shared_memory_object.hpp 80.00% <100.00%> (+13.33%) ⬆️
...six_wrapper/shared_memory_object/shared_memory.hpp 100.00% <100.00%> (ø)
...oofs/source/posix_wrapper/shared_memory_object.cpp 82.50% <100.00%> (+1.67%) ⬆️
...lude/iceoryx_posh/internal/mepoo/mepoo_segment.inl 95.34% <100.00%> (-0.11%) ⬇️
...de/iceoryx_posh/internal/mepoo/segment_manager.inl 86.66% <100.00%> (ø)
iceoryx_posh/source/roudi/port_manager.cpp 78.38% <0.00%> (-0.33%) ⬇️
iceoryx_hoofs/source/posix_wrapper/timer.cpp 63.90% <0.00%> (+0.82%) ⬆️
... and 1 more

Copy link
Member

@elBoberido elBoberido left a 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

@elfenpiff
Copy link
Contributor Author

elfenpiff commented Feb 1, 2022

@elBoberido I created the pull request #1061 and a separate issue #1059 which only handles the C++17 perms enum class.

@elfenpiff elfenpiff force-pushed the iox-#1036-in-place-creation-for-shared-memory branch 3 times, most recently from d5bb04a to 598fd56 Compare February 2, 2022 10:06
@elfenpiff elfenpiff mentioned this pull request Feb 3, 2022
21 tasks
@elfenpiff elfenpiff force-pushed the iox-#1036-in-place-creation-for-shared-memory branch from 598fd56 to beb4da1 Compare February 4, 2022 10:24
Copy link
Member

@elBoberido elBoberido left a 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

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);
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor

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

@elBoberido elBoberido added the refactoring Refactor code without adding features label Feb 4, 2022
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>
@elfenpiff elfenpiff force-pushed the iox-#1036-in-place-creation-for-shared-memory branch from beb4da1 to a085d46 Compare February 7, 2022 09:48
…s, cleanup shm file handle one failure, unlink shm when owned on failure

Signed-off-by: Christian Eltzschig <me@elchris.org>
elBoberido
elBoberido previously approved these changes Feb 7, 2022
Copy link
Member

@elBoberido elBoberido left a 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.

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>
@elfenpiff elfenpiff force-pushed the iox-#1036-in-place-creation-for-shared-memory branch from a1b44a8 to eee632b Compare February 7, 2022 12:36
elBoberido
elBoberido previously approved these changes Feb 7, 2022
Signed-off-by: Christian Eltzschig <me@elchris.org>
…pipe

Signed-off-by: Christian Eltzschig <me@elchris.org>
@elfenpiff elfenpiff merged commit 947a850 into eclipse-iceoryx:master Feb 8, 2022
@elfenpiff elfenpiff deleted the iox-#1036-in-place-creation-for-shared-memory branch February 8, 2022 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Refactor code without adding features
Projects
None yet
3 participants