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 manual bindings for GString. #317

Merged
merged 1 commit into from Apr 22, 2018

Conversation

Projects
None yet
4 participants
@tmiasko
Copy link
Contributor

tmiasko commented Apr 17, 2018

C API follows chaining pattern which is replicated in Rust version as
well. It is manually written instead of being auto-generated to avoid
unnecessary copies (generator doesn't know that the same thing is being
returned back as was provided to a function as a mutable argument).


impl Eq for String {}

impl convert::AsRef<[u8]> for String {

This comment has been minimized.

@sdroege

sdroege Apr 17, 2018

Member

Why not also for &str?

This comment has been minimized.

@tmiasko

tmiasko Apr 17, 2018

Author Contributor

Content is not guaranteed to be UTF-8. This is essentially the only reason.

This comment has been minimized.

@sdroege

sdroege Apr 17, 2018

Member

GLib strings must be UTF-8, but that of course makes the truncate function unsafe.

This comment has been minimized.

@tmiasko

tmiasko Apr 17, 2018

Author Contributor

I don't think that GString have such requirement, in fact documentations states that they are "typically UTF-8", which strongly suggested that not always. They certainly don't enforce it anywhere, and you can insert arbitrary bytes with prepend_len, append_len etc., not to mention that whole struct is intended to be public.

This comment has been minimized.

@sdroege

sdroege Apr 17, 2018

Member

Having some convenience functions for converting to std::String would be useful then at least.

This comment has been minimized.

@tmiasko

tmiasko Apr 17, 2018

Author Contributor

Currently you can do that with as_ref() followed by a variant of from_utf8. Not that bad, but I am open for suggestions. TryFrom / TryInto traits would be good fit, but they are still unstable.

This comment has been minimized.

@sdroege

sdroege Apr 17, 2018

Member

I was thinking of proxying the &str API... but you could as well just implement Deref<Target=[u8]> and have it all for free

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Apr 17, 2018

Looks generally ok, but why? Rust String is more powerful :)
Is there API that passes GStrings around?

@tmiasko

This comment has been minimized.

Copy link
Contributor Author

tmiasko commented Apr 17, 2018

Yes, D-Bus related API uses GStrings.

@tmiasko

This comment has been minimized.

Copy link
Contributor Author

tmiasko commented Apr 17, 2018

Another revision, with following changes:

  • Implements Deref<Target=[u8]>
  • to_str and to_string_lossy based on interface in CStr
  • Eq is implemented only for the same type.

I don't envision this type will get a lot of use. As you mentioned Rust String type will be superior in almost every aspect, apart from the fact that some C API require this particular type in a way that would be somewhat inefficient to directly replace with Rust String.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Apr 18, 2018

Fine with me then :) Can be changed in the future if needed

Tomasz Miąsko
Provide manual bindings for GString.
C API follows chaining pattern which is replicated in Rust version as
well. It is manually written instead of being auto-generated to avoid
unnecessary copies (generator doesn't know that the same thing is being
returned back as was provided to a function as a mutable argument).

GString is intended to represent textual content, but there is no
guarantee that data it contains will valid UTF-8. For convenience two
conversion methods are provided to_str and to_string_lossy, based
on interface in Rust CStr.
@tmiasko

This comment has been minimized.

Copy link
Contributor Author

tmiasko commented Apr 20, 2018

Rebased.

@tmiasko tmiasko referenced this pull request Apr 21, 2018

Closed

Generate D-Bus types. #110

2 of 2 tasks complete
@tmiasko

This comment has been minimized.

Copy link
Contributor Author

tmiasko commented Apr 22, 2018

@EPashkin, @GuillaumeGomez, any additional comments, or ready to merge?

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Apr 22, 2018

LGFM

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Apr 22, 2018

Just like @EPashkin said. :)

@GuillaumeGomez GuillaumeGomez merged commit 20c6402 into gtk-rs:master Apr 22, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.