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

Use a real Volatile struct #288

Closed
gkoz opened this Issue Oct 25, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@gkoz
Copy link
Member

gkoz commented Oct 25, 2016

We might want to import Volatile from the volatile crate in the sys crates. It doesn't have a constructor though, so @antoyo you might still not be entirely happy with it.
gtk-rs/sys#25 (comment)

@antoyo

This comment has been minimized.

Copy link
Member

antoyo commented Oct 26, 2016

I am not sure why we need a wrapper for the ref_count volatile fields.
Could you please explain me why?

I believe most of the time, the user of the API will not change this value directly: it will be updated by the g_object_ref function.
And if an update is needed, perhaps write_volatile from the standard library could be used.

Moreover, it seems to be used only in the sys crates, right?

Am I missing something?

@gkoz

This comment has been minimized.

Copy link
Member Author

gkoz commented Oct 26, 2016

Generally, it seemed a good idea to statically prevent inappropriate accesses to volatile fields at the sys crates level. The higher level crates so far haven't touched such fields because the API functions take care of that indeed.

I suspect mem::swap can subvert that restriction anyway so it may be a bit pointless. Still, like in the privacy issue it seems wrong to just ignore the volatile thing. Do you think we can find some middle ground here?

@gkoz

This comment has been minimized.

Copy link
Member Author

gkoz commented Oct 26, 2016

I guess changing Volatile to be a transparent wrapper could provide some notice while leaving the users with responsibility to actually do the right thing. Something like

#[repr(C)]
pub struct Volatile<T> {
    pub volatile: T,
}
@antoyo

This comment has been minimized.

Copy link
Member

antoyo commented Oct 26, 2016

Having a public field would help me.

I have another idea, but I am not sure if it is good thought and it only works for the ref_count field.

My issue is that I want to create a const struct with a ref_count of -1 (meaning not ref counted).
So perhaps we could create an enum like the following:

enum RefCount {
    NoRefCount,
    RefCount(Volatile),
}

I don't think creating a const struct with a ref count make sense, so the Volatile field could stay abstract.

@gkoz

This comment has been minimized.

Copy link
Member Author

gkoz commented Oct 26, 2016

Eh, I don't think this kind of special treatment for some fields in the sys crates is going to work.

@antoyo

This comment has been minimized.

Copy link
Member

antoyo commented Nov 2, 2016

Ok, then if we could make the field public, this code would work.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Apr 28, 2018

Volatible break ABI on at minimum on Windows 32, so it was removed in #579 completely.

@EPashkin EPashkin closed this Apr 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.