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

Provide comparison operators for rust::Str and rust::String #57

Closed
dtolnay opened this issue Mar 2, 2020 · 2 comments · Fixed by #531
Closed

Provide comparison operators for rust::Str and rust::String #57

dtolnay opened this issue Mar 2, 2020 · 2 comments · Fixed by #531

Comments

@dtolnay
Copy link
Owner

dtolnay commented Mar 2, 2020

The C++ types should come with operators for ==, !=, <, >, <=, >=.

@dtolnay dtolnay changed the title Provide comparison operators for Str and String Provide comparison operators for rust::Str and rust::String May 13, 2020
@alexmaco
Copy link

While looking into adding this, I noticed that a few trivial functions of alloc::String are routed via FFI, e.g. cxxbridge1$string$len for String::len. So long as Vec has a guaranteed memory layout, and String is a Vec<u8>, then it should be possible to get the length out of rust::String::repr[1], and similarly for the pointer.

I bring this up because, if this would work reliably, then all these operators could be implemented with just an equality and a comparison function that operates on 2 string slices (technically two pointer + length pairs, since &str is not FFI safe), in addition to removing the accessors mentioned above.

alloc::String is not yet marked as repr_transparent, pending rust-lang/rust#70473.

Do you think having repr_transparent on String is really a requirement in practice for this case, or should this go ahead before the issue above is implemented ? Because personally, I can't see how the layout of String could differ from Vec<u8> in practice, given its implementation.

@dtolnay
Copy link
Owner Author

dtolnay commented Nov 18, 2020

Vec does not have a guaranteed layout.

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

Successfully merging a pull request may close this issue.

2 participants