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

Add: CompactCowStr #371

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add: CompactCowStr #371

wants to merge 3 commits into from

Conversation

aobatact
Copy link
Contributor

This pull request aims to add a new struct called CompactCowStr to incorporate Cow for CompactString.
This proposal has been previously discussed in issue #223.

Question

  • Reuse of Repr:
    CompactCowStr reuses Repr, which may potentially impact CompactString.
    If Cow flag of LastUtf8Char leaked to CompactString, this might cause UB.

  • Methods:
    Which methods equivalent to those in CompactString are necessary for CompactCowStr?

  • Inlining:
    For the new method, should we inline it?

  • Name Discussion:
    Previous pull request suggested the name CompactCow, but considering its relationship to String, I have changed it to CompactCowStr.

@aobatact
Copy link
Contributor Author

miri is failing with "unknown unstable option: miri-check-number-validity".
It seems the CI needs to be fixed?

@aobatact aobatact marked this pull request as ready for review March 31, 2024 13:40
@NobodyXu
Copy link
Contributor

I think that miri cmdline option might be removed in the nightly, so it probably needs to also be removed from CI?

@aobatact
Copy link
Contributor Author

aobatact commented Apr 29, 2024

Fixed CI in #373

@ParkMyCar
Copy link
Owner

Hey @aobatact, sorry for the delay and thank you for the PR! I took a quick look and I think we're on the right path, but I've paged out a decent amount of the context here. I have a long train ride this weekend which I'll use to dive in deep here.

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.

None yet

3 participants