Skip to content

&mut [u8] support #452

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

Closed
wants to merge 7 commits into from
Closed

&mut [u8] support #452

wants to merge 7 commits into from

Conversation

2xsaiko
Copy link

@2xsaiko 2xsaiko commented Nov 11, 2020

This seems to work, but I have no idea if it's the correct way to implement it.

Closes #451.

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks, this mostly looks good except for the API of rust::MutSlice. The C++ API is going to need some thought because e.g. copying a &mut [T] is UB so a copy constructor probably can't exist.

It may also be worth considering rust::Slice<const T> for the const one and rust::Slice<T> for the non-const one.

@2xsaiko
Copy link
Author

2xsaiko commented Nov 11, 2020

I mean, you could just copy it by getting the data pointer and constructing another slice from it anyway. But yeah, technically that is UB so maybe the copy constructor shouldn't exist.

Oh yeah, being able to reuse the Slice type for both mutable and immutable slices would be pretty nice.

@2xsaiko
Copy link
Author

2xsaiko commented Nov 11, 2020

Hmm, the tests are failing because rust::repr::PtrLen has a const void* as the pointer type which it's trying to cast to uint8_t*. How should this be fixed?

Edit: also not seeing how to have the copy constructor only be available for Slice<const T> after #453. I tried messing around with std::enable_if/SFINAE but this is beyond my limited C++ knowledge :P

@dtolnay
Copy link
Owner

dtolnay commented Nov 19, 2020

Hmm, the tests are failing because rust::repr::PtrLen has a const void* as the pointer type which it's trying to cast to uint8_t*. How should this be fixed?

Looks like a job for https://en.cppreference.com/w/cpp/language/const_cast. Alternatively maybe if PtrLen were to contain void* it would work in both cases?

Edit: also not seeing how to have the copy constructor only be available for Slice<const T> after #453. I tried messing around with std::enable_if/SFINAE but this is beyond my limited C++ knowledge :P

Yeah something with enable_if. I'm not a C++ pro either, one of us will just need to poke on it until it works. -.-

@dtolnay dtolnay added the wip work in progress PR label Nov 19, 2020
@2xsaiko
Copy link
Author

2xsaiko commented Nov 22, 2020

Well, this should work now. At least the tests pass, I can't check it with real code yet though because of #496.

@2xsaiko
Copy link
Author

2xsaiko commented Nov 23, 2020

I can't get the enable_if on the slice constructors to work for some reason, and at this point I think I tried 7 different ways of doing it. It either works for both mutable and const slices or not at all. At this point I'd just leave it since C++ is already unsafe (even if that constructor was gone, you could still trivially copy mutable slices) unless you really want this check.

Everything else seems to work now though, after testing it with the project I'm using cxx with.

@dtolnay dtolnay mentioned this pull request Nov 24, 2020
@dtolnay dtolnay closed this in #502 Nov 24, 2020
@dtolnay dtolnay removed the wip work in progress PR label Feb 7, 2021
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.

Allow usage of &mut [u8] in FFI
2 participants