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

remake vector_set from inheritance from underlying container to aggre… #488

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

AntonYudintsev
Copy link
Contributor

…gation

this makes it safer, as it disallows some of changes that could violate sorted contract it also saves at least sizeof(void*) on most common comparators (due to empty struct optimization in compressed_pair)

con: it explicitly requires container class to follow vector API (almost all functions are exposed in vector_set)

to consider: remove non const front(), back(), at(), [] access. Those are not safe either, left them only for compatibility

…gation

this makes it safer, as it disallows some of changes that could violate sorted contract
it also saves at least sizeof(void*) on most common comparators (due to empty struct optimization in compressed_pair)

con: it explicitly requires container class to follow vector API (almost all functions are exposed in vector_set)

to consider: remove non const front(), back(), at(), [] access.
Those are not safe either, left them only for compatibility
@grojo-ea
Copy link
Contributor

Hello,

We actually have an internal change in progress (unreleased as of this writing) to reduce the size of vector_(multi)set and vector_(multi)map that I think will conflict with this change so I'm going to hold off on the PR for now.

@grojo-ea
Copy link
Contributor

grojo-ea commented Jul 5, 2023

Hello again,

Version 3.21.12 (#510) has the changes we had in progress that I mentioned above. Let me know if you want to try to rebase on the new version to try to get some of the API/inheritance changes reviewed or if we should close this PR instead.

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.

2 participants