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

pub type CFMutableDictionaryRef = *const __CFDictionary; #149

Closed
jrmuizel opened this issue Jan 25, 2018 · 14 comments
Closed

pub type CFMutableDictionaryRef = *const __CFDictionary; #149

jrmuizel opened this issue Jan 25, 2018 · 14 comments

Comments

@jrmuizel
Copy link
Collaborator

@jrmuizel jrmuizel commented Jan 25, 2018

Should this be *mut __CFDictionary?

@jrmuizel
Copy link
Collaborator Author

@jrmuizel jrmuizel commented Jan 25, 2018

Also we have impl TCFTypeRef for *const T but not for *mut T. I ran into this when trying to update security-framework to a new version of core-foundation.

@faern
Copy link
Contributor

@faern faern commented Jan 26, 2018

@jrmuizel All ref types are *const. I just changed the remaining *mut __CFError into *const to have all of them the same.

Why do you need/want some of them to be *mut? EDIT: What error did you run into?

@faern
Copy link
Contributor

@faern faern commented Jan 26, 2018

Yes, TCFTypeRef is only implemented for *const T on purpose. Since it converts to and from *const c_void.

@jrmuizel
Copy link
Collaborator Author

@jrmuizel jrmuizel commented Jan 26, 2018

Why *const? Since we're going to be mutating at least the refcount and sometimes the whole object pointed to by the ref type shouldn't they be *mut?

@faern
Copy link
Contributor

@faern faern commented Jan 26, 2018

@jrmuizel Every ref raw pointer definition in the crate, except for CFError, was already *const. And it seemed to have worked fine, since no one had reported an issue. So I thought changing one instead of changing all the others was both easier and probably more correct.

We never mutate any data behind the pointers inside this crate, or in Rust at all. We just pass the pointers to CFRetain and CFRelease for example and they don't care if the pointer is mutable or not.

@faern
Copy link
Contributor

@faern faern commented Jan 26, 2018

These C functions actually want *const pointers in our FFI bindings to them: https://docs.rs/core-foundation-sys/0.5.0/core_foundation_sys/base/fn.CFRelease.html

@faern
Copy link
Contributor

@faern faern commented Jan 26, 2018

@jrmuizel What practical problem does it cause to have *const? What problem did you run into in security-framework?

@jrmuizel
Copy link
Collaborator Author

@jrmuizel jrmuizel commented Jan 26, 2018

All of the ref types here are mut. Do we have a justification for changing them to *const?

@faern
Copy link
Contributor

@faern faern commented Jan 26, 2018

I'm not sure. Because all of core-foundation use const and the ffi functions take const pointers, that's the only reason I can come up with. Maybe this crate should have everything *mut. I don't think it matters very much as long as everyone do the same. Maybe the person who started using *const all over this crate has a reason? :)

@faern
Copy link
Contributor

@faern faern commented Jan 26, 2018

Iff we come to the conclusion that it does not matter; then I would argue that the reason to change in security-framework is because this crate has had *const for most types for quite a while, it's already made consistent and is ready for release. While on the other hand, security-framework will need a lot of updating anyway so changing to *const could just be done at the same time.

@jrmuizel
Copy link
Collaborator Author

@jrmuizel jrmuizel commented Jan 26, 2018

@sfackler would you object to changing security-framework to use *const for it's ref types instead of *mut?

@Gankra
Copy link

@Gankra Gankra commented Jan 26, 2018

(asked on IRC to toss in my two cents here)

There's no language-level significance between *mut and *const that should matter here (*const is covariant, but that shouldn't matter); they're basically intended as lints/hints for correct usage. In that regard *mut is more "correct" here as it's honest about the mutations that will be performed.

That said, I personally don't think this hint is important enough to refactor your whole codebase over, but I believe sfackler cares about it more strongly.

@sfackler
Copy link
Contributor

@sfackler sfackler commented Jan 26, 2018

I like to match the C type definitions exactly, even when they're not const-correct.

I'm going to do a big breaking release for security-framework soon so if there are changes needed on the securyt-framework side that's fine.

@jdm
Copy link
Member

@jdm jdm commented Jan 26, 2018

@jrmuizel jrmuizel closed this Jan 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.