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

Add Quark type #362

Merged
merged 2 commits into from Jul 28, 2018

Conversation

Projects
None yet
3 participants
@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Jul 28, 2018

No description provided.

}
}

pub fn intern_static_string(s: &str) -> Option<String> {

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jul 28, 2018

Author Member

I'm just worry about this implementation: do we have ownership on the returned string or not? If so, we'll have issues because we're not supposed to free it...

This comment has been minimized.

@EPashkin

EPashkin Jul 28, 2018

Member

from_glib_none IMHO is right, but this function don't make sense in rust and better not implemented.
Also it already in https://github.com/gtk-rs/glib/blob/master/src/auto/functions.rs#L614 and better ignored with intern_string

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jul 28, 2018

Author Member

Sounds good to me!

This comment has been minimized.

@sdroege

sdroege Jul 28, 2018

Member

The returned string here is the same as below in to_string(). Static string slice would be correct

But you can't bind the static variant, only the other. And both intern functions are not very useful in Rust

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:quark branch from e058da4 to bbf3dce Jul 28, 2018

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jul 28, 2018

Removed the two functions.


pub fn to_string(&self) -> Option<String> {
unsafe {
from_glib_none(ffi::g_quark_to_string(self.to_glib()))

This comment has been minimized.

@sdroege

sdroege Jul 28, 2018

Member

The return value has static lifetime and is never NULL. You can directly use CStr here and return a static string slice


pub fn from_static_string(s: &'static str) -> Quark {
unsafe {
from_glib(ffi::g_quark_from_static_string(s.to_glib_none().0))

This comment has been minimized.

@sdroege

sdroege Jul 28, 2018

Member

This is wrong. The string passed to C must have static lifetime but due to conversion it doesn't here, is only in this scope.

We can't bind this function in a meaningful way due to the difference between C and Rust strings. The trailing 0

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jul 28, 2018

Author Member

So I just remove it as well?

This comment has been minimized.

@sdroege

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jul 28, 2018

Author Member

Ok!

impl Quark {
pub fn from_string(s: &str) -> Quark {
unsafe {
from_glib(ffi::g_quark_from_string(s.to_glib_none().0))

This comment has been minimized.

@sdroege

sdroege Jul 28, 2018

Member

If you want to prevent a copy, you could make this one here use to_glib_full() and have it call the static variant of the function. Same overall behaviour but one less copy

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:quark branch from bbf3dce to 2f80995 Jul 28, 2018

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jul 28, 2018

@GuillaumeGomez Please remove generated intern_(static_)?string too

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:quark branch from 2f80995 to 83d8001 Jul 28, 2018

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jul 28, 2018

@EPashkin They're already removed no? Or are you talking about something outside of Quark?

use std::ffi::CStr;

#[repr(C)]
pub struct Quark(ffi::GQuark);

This comment has been minimized.

@sdroege

sdroege Jul 28, 2018

Member

Now also add Eq and PartialEq and Clone and Copy impls :) You can derive them, those are the correct ones

Otherwise all good and perfect

This comment has been minimized.

@sdroege

sdroege Jul 28, 2018

Member

I guess Hash and the Ord ones too

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jul 28, 2018

Author Member

Ok! ;)

@EPashkin

This comment has been minimized.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jul 28, 2018

@EPashkin If I remove them, we won't have access to the intern string anywhere. If it's what you want then I'll just remove them.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jul 28, 2018

That API makes no sense from Rust, you can remove :)

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jul 28, 2018

@GuillaumeGomez Yes, I think that both function unusable in rust

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jul 28, 2018

I did. I think it's a first for me to remove things instead of adding ones haha.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:quark branch from 83d8001 to d7fd19c Jul 28, 2018

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jul 28, 2018

Updated.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jul 28, 2018

Perfect, I'm offline again :)

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jul 28, 2018

👍

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jul 28, 2018

Thanks to both of you for your reviews! :)

@GuillaumeGomez GuillaumeGomez merged commit 2326b65 into gtk-rs:master Jul 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:quark branch Jul 28, 2018

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.