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

Generalized Vec, implemented Vec::transmute_buffer. #430

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

Conversation

JohnScience
Copy link

No description provided.

@JohnScience
Copy link
Author

Ready for the review!

@JohnScience
Copy link
Author

@Dirbaio

src/vec.rs Outdated Show resolved Hide resolved
}
}

const fn as_ptr(&self) -> *const T {
Copy link
Member

Choose a reason for hiding this comment

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

you can implement Deref and DerefMut into [T; N] so all the usual slice methods are instantly available, without you having to wrap them.

Copy link
Author

Choose a reason for hiding this comment

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

you can implement Deref and DerefMut into [T; N] so all the usual slice methods are instantly available, without you having to wrap them.

Done. And I kept the methods to provide const alternatives.

Copy link
Member

Choose a reason for hiding this comment

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

Does this actually have to be const? If not, may as well remove it again, along with as_mut_ptr and move the cast back.

@Dirbaio
Copy link
Member

Dirbaio commented Jan 1, 2024

i'm not 100% convinced we should have transmute_buffer in the public API, it's quite easy to misuse and a bit niche. With the alignment generic, it should be possible for user code to safely do any transmuting on its own.

@JohnScience
Copy link
Author

JohnScience commented Jan 2, 2024

i'm not 100% convinced we should have transmute_buffer in the public API, it's quite easy to misuse and a bit niche. With the alignment generic, it should be possible for user code to safely do any transmuting on its own.

In defense of transmute buffer, this method is the primary reason for addition of the alignment type parameter.

In order to avoid the pollution of the namespace, this method is provided only for T=u8.

impl<const N: usize, A> Vec<u8, N, A> {
 // fn transmute_buffer
}

I'll try elaborate on the choice here. In order to make the transmutation in user code safe, it has to either expose this function (transmute_buffer) or expose the buffer. Otherwise, it'd be impossible/hard to do safely. And I believe that hiding this implementation detail is quite important.

@JohnScience
Copy link
Author

Ready for review!

src/vec.rs Outdated Show resolved Hide resolved
src/vec.rs Outdated Show resolved Hide resolved
@JohnScience
Copy link
Author

@reitermarkus Fixed

src/vec.rs Outdated Show resolved Hide resolved
@JohnScience
Copy link
Author

@reitermarkus Fixed.

@Dirbaio
Copy link
Member

Dirbaio commented Jan 22, 2024

I'll try elaborate on the choice here. In order to make the transmutation in user code safe, it has to either expose this function (transmute_buffer) or expose the buffer.

We already expose the buffer with .as_ptr() / .as_mut_ptr(), so it's already possible to do transmute_buffer from user code soundly.

src/vec.rs Outdated Show resolved Hide resolved
src/vec.rs Outdated Show resolved Hide resolved
@JohnScience
Copy link
Author

JohnScience commented Jan 22, 2024

I'll try elaborate on the choice here. In order to make the transmutation in user code safe, it has to either expose this function (transmute_buffer) or expose the buffer.

We already expose the buffer with .as_ptr() / .as_mut_ptr(), so it's already possible to do transmute_buffer from user code soundly.

Another reason why I'm not particularly happy with leaving this to the user is that with the stabilization of std::intrinsics::transmute_unchecked we will be able to no longer rely on the optimization of the unnecessary copy created with std::mem::transmute_copy. However, we won't be able to change the code of the library users if we delegate this responsibility to them.

Later on, we'll be able to deprecate the method and one day eventually even remove it if there's a need for a major update (= other breaking changes).

EDIT:

I thought more about it and I think it's alright to remove the method because this fix can be applied with Clippy.

CHANGELOG.md Outdated Show resolved Hide resolved
@reitermarkus
Copy link
Member

@JohnScience, can you rebase this?

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