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

Dev twon allocator pointer #103

Merged
merged 20 commits into from
Nov 8, 2023
Merged

Conversation

Twon
Copy link
Collaborator

@Twon Twon commented Nov 1, 2023

No description provided.

@Twon Twon requested a review from jbcoe November 1, 2023 11:34
@Twon
Copy link
Collaborator Author

Twon commented Nov 1, 2023

This could use tests. Something like https://www.boost.org/doc/libs/1_83_0/doc/html/interprocess/offset_ptr.html would be idea to demonstrate this works.

Copy link

codecov bot commented Nov 1, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bfbd46e) 100.00% compared to head (dfe53a7) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #103   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          474       474           
=========================================
  Hits           474       474           
Files Coverage Δ
indirect.h 100.00% <100.00%> (ø)
polymorphic.h 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jbcoe
jbcoe previously approved these changes Nov 1, 2023
@jbcoe
Copy link
Owner

jbcoe commented Nov 1, 2023

Testing this is non-trivial so I'd defer for now.

Copy link
Collaborator

@Ukilele Ukilele left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least line 58 T* mem = allocator_traits::allocate(alloc_, 1); will fail to compile as we still expect a T* and not a allocator_traits::pointer.

@Twon
Copy link
Collaborator Author

Twon commented Nov 1, 2023

Testing this is non-trivial so I'd defer for now.

I'm going to add a branch using Conan to fetch boost so that I can at least test this again boost::interprocess::offset_ptr to flush out all of the issues like the one @Ukilele has raised. We won't check in these changes, but we can at least verify we've updated all of the places that should be using pointer

@jbcoe
Copy link
Owner

jbcoe commented Nov 8, 2023

Does this PR need accompanying wording changes? If so, we need these very soon.

@Twon
Copy link
Collaborator Author

Twon commented Nov 8, 2023

Does this PR need accompanying wording changes? If so, we need these very soon.

I believe the wording in the draft is now updated. Please take a look and make sure you are happy with it. We should get this checked in but this still needs further testing against boost interprocess allocators there are compiler errors to resolve in the test branch.

@Twon Twon requested a review from jbcoe November 8, 2023 02:10
jbcoe
jbcoe previously approved these changes Nov 8, 2023
Copy link
Owner

@jbcoe jbcoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work. Approved with minor tweaks to whitespace.

DRAFT.md Outdated Show resolved Hide resolved
DRAFT.md Outdated Show resolved Hide resolved
DRAFT.md Show resolved Hide resolved
@Twon Twon merged commit 403e264 into jbcoe:main Nov 8, 2023
21 checks passed
@Twon Twon deleted the dev-twon-allocator-pointer branch November 8, 2023 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants