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

fix: retain not set len if predicate panics #413

Merged
merged 2 commits into from
Dec 29, 2024

Conversation

ryota2357
Copy link
Contributor

The current implementation of CompactString::retain breaks len if the predicate passed as retain method argument panics during iteration.

Please see: rust-lang/rust#78498

For this problem, I did:

  • Added test alloc::String have.
  • Fix retain method implementation.

@ryota2357
Copy link
Contributor Author

ryota2357 commented Dec 21, 2024

CI using nightly rust failed because of dangling_pointers_from_temporaries.

  • src/test.rs:1225
  • src/test.rs:1242

I couldn't understand why 'Fuzz' and 'example - diesel' failed.

@ryota2357
Copy link
Contributor Author

ryota2357 commented Dec 21, 2024

I added #[allow(dangling_pointers_from_temporaries)] to failed test cases.
This lint is set at milestone 1.84.0 (rust-lang/rust#128985)

@ParkMyCar
Copy link
Owner

This is fantastic @ryota2357, thank you for the fix!

Apologies for the various clippy and test changes you had to make, I just merged #414 and #415 that separately fixed these issues so I removed the last three commits from this PR. I also appreciate how you made those changes in separate commits, made it super easy to revert!

I plan to release a v0.9 soon, but will also backport this fix to v0.8 and v0.7

@ParkMyCar ParkMyCar merged commit 0e48d4a into ParkMyCar:main Dec 29, 2024
27 checks passed
ParkMyCar pushed a commit that referenced this pull request Dec 29, 2024
@ryota2357 ryota2357 deleted the fix-retain branch December 30, 2024 23:51
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