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

Clean up FastStr::new_inline_impl #9

Closed
wants to merge 1 commit into from
Closed

Clean up FastStr::new_inline_impl #9

wants to merge 1 commit into from

Conversation

saethlin
Copy link

Bug fixes and new features should include tests.

There are no tests, so I'm a bit confused by this.

Motivation

MaybeUninit::uninit().assume_init() to create a [u8; N] is UB. It is always UB, immediately, because u8 must be initialized. Code using this pattern tends to not cause issues currently because LLVM is not very good at tracking initialization, but the UB in this crate is an obstacle for assessing users of this crate.

Solution

I just used initialized bytes, as is done everywhere else. Benchmarks do not indicate any measurable performance change, which is expected for such a small buffer. The alternative is to use MaybeUninit in Repr::Inline, but that would require a lot more code change for no measurable performance impact.

I also swapped in copy_nonoverlapping; the copied ranges are guaranteed to be non-overlapping because the destination buffer is a local and the source buffer is a passed-in pointer.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@saethlin
Copy link
Author

Oh, you have a CLA. Feel free to apply this patch yourselves. It would be nice to have.

@saethlin saethlin closed this Apr 19, 2024
@PureWhiteWu
Copy link
Contributor

Hi, sorry for the late reply!
Thank you very much for your detailed explanation! I think you are right, although this will not cause any issue now, but it's always UB here, and the performance loss can be ignored in such a small buffer here.
I will submit a PR to fix this!

PureWhiteWu added a commit that referenced this pull request Apr 22, 2024
PureWhiteWu added a commit that referenced this pull request Apr 22, 2024
@PureWhiteWu
Copy link
Contributor

Fixed in #10 .

Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants