[RFC] Make the implementations of FromVoid more generic which gives better ergonomics #143

jrmuizel · 2018-01-14T14:22:05Z

This is probably a breaking change so would be good to include with 0.5 changes.

This change is

nox · 2018-01-14T16:07:32Z

Can you reorder the commits, or at least not put the bumping commit in the middle of the others? Thanks.

jrmuizel · 2018-01-14T19:28:02Z

All but the last of the commits are from #132

jrmuizel · 2018-01-14T20:30:35Z

I needed to drop the re-implementation of get_values() because we don't have a way to implement it only for types that that can be transmuted from const void *

nox · 2018-01-14T20:37:41Z

@jrmuizel Oh never mind then, didn't realise this was blocked on another PR.

faern · 2018-01-15T00:46:24Z

Would it maybe be possible to remove FromVoid completely and use TCFTypeRef in it's place? They are very similar, but I have not checked if it would be completely compatible, but I think so.

TCFTypeRef was meant as a trait for all the ref types from the sys crate which actually all are *const Foo. And I guess a CFArray is only meant to contain CF types? Then it would not just make type sense, but also meaningful sense to express the generic of the array with that trait.

faern · 2018-01-15T00:46:47Z

core-foundation/src/array.rs

-        x
+unsafe impl<T> FromVoid for *const T {
+    unsafe fn from_void(x: *const c_void) -> Self {
+        mem::transmute(x)


This can be done without transmute, just with x as *const T, like I do in TCFTypeRef::from_void_ptr.

faern · 2018-01-15T00:56:57Z

core-foundation/src/array.rs

-        TCFType::wrap_under_get_rule(mem::transmute(x))
+unsafe impl<T: TCFType> FromVoid for T where T::Ref: FromVoid {
+    unsafe fn from_void(x: *const c_void) -> Self {
+        TCFType::wrap_under_get_rule(T::Ref::from_void(x))


Is it maybe a bit risky to assume any void pointer should be wrapped under the get rule? I guess that depend on how one obtain the pointer? I might be mistaken how it's used, but that is my impression.

A possibly safer way would be to force the user to do what I do in downcast: https://github.com/servo/core-foundation-rs/pull/143/files#diff-e49ae9facf583e1f0d73ba355079e544R199
Where they must convert their void pointer to an appropriate ref type and then apply one of the ordinary constructors wrap_under_get_rule or wrap_under_create_rule.

I'm not sure I understand how that would work. from_void is only used internally in array.rs and is unsafe. My understanding is that CFArray contents would always be accessed with wrap_under_get_rule.

Is it maybe a bit risky to assume any void pointer should be wrapped under the get rule? I guess that depend on how one obtain the pointer? I might be mistaken how it's used, but that is my impression.

@faern It's never "more" unsafe to wrap anything under the get rule. Worst case, it will retain a value too much and leak it. I wouldn't worry about this too much for now, given we have no notion of returning a borrow of an already-retained value.

True, it's not unsafe to leak. Bad wording on my part I guess. But maybe the documentation should at least state that the retain count will be incremented on this operation? :)

I wonder if we should actually change CFArray to return an iterator of references instead of values...

faern · 2018-01-15T01:00:09Z

@nox That other PR is, however, not blocked. It's ready for merge. And I just rebased it to make the version bump the last commit :)

jrmuizel · 2018-01-15T01:28:32Z

CFArrays can be used for types other than CFTypes. For example, CGFontCopyTableTags returns a CFArray of u32.

bors-servo · 2018-01-18T01:07:12Z

☔ The latest upstream changes (presumably #141) made this pull request unmergeable. Please resolve the merge conflicts.


        Change CFErrorRef to *const ptr instead of *mut


        Add TCFTypeRef trait


        Change ConcreteTypeRef to an associated value


        Remove deprecated methods


        Declare CFFileDescriptor with declare_TCFType macro


        Bump version to 0.5.0


        Clean up my own non-rustfmt style


        Add the ability to borrow values from the array.

This uses borrowed values from the array to avoid having to Retain/Release during iteration. It also implements borrowing for all TCFType.


        Auto merge of #147 - jrmuizel:borrow, r=jdm

Add the ability to borrow values from the array. This is a replacement for #143. It allows iteration while avoiding the reference count churn of the previous approach. It uses a TCFTypeBorrow (could probably use a better name) to hold a sort of reference to the array data. I'm interested in people's thoughts and opinions on this approach.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/core-foundation-rs/147)

jrmuizel force-pushed the jrmuizel:cfarray-more-type branch from 4988599 to cc141bb Jan 14, 2018

faern reviewed Jan 15, 2018

View changes

jrmuizel force-pushed the jrmuizel:cfarray-more-type branch from cc141bb to 0c2cf57 Jan 15, 2018

jdm approved these changes Jan 17, 2018

View changes

faern added 7 commits Nov 26, 2017

Change CFErrorRef to *const ptr instead of *mut

b35cfc0

Add TCFTypeRef trait

753b5ce

Change ConcreteTypeRef to an associated value

36a3fad

Remove deprecated methods

e42ea7b

Declare CFFileDescriptor with declare_TCFType macro

7c74a6c

Bump version to 0.5.0

Loading status checks…

528ff79

Clean up my own non-rustfmt style

Loading status checks…

63a43bb

jrmuizel mentioned this pull request Jan 23, 2018

Add the ability to borrow values from the array. #147

Merged

Add the ability to borrow values from the array.

Loading status checks…

922da73

This uses borrowed values from the array to avoid having to Retain/Release during iteration. It also implements borrowing for all TCFType.

jrmuizel force-pushed the jrmuizel:cfarray-more-type branch from 0c2cf57 to 922da73 Jan 24, 2018

jrmuizel closed this Jan 24, 2018

servo / core-foundation-rs

[RFC] Make the implementations of FromVoid more generic which gives better ergonomics #143

[RFC] Make the implementations of FromVoid more generic which gives better ergonomics #143

jrmuizel commented Jan 14, 2018 •

edited by larsbergstrom

nox commented Jan 14, 2018

jrmuizel commented Jan 14, 2018

jrmuizel commented Jan 14, 2018

nox commented Jan 14, 2018

faern commented Jan 15, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

faern commented Jan 15, 2018

jrmuizel commented Jan 15, 2018

bors-servo commented Jan 18, 2018

servo / core-foundation-rs

Join GitHub today

[RFC] Make the implementations of FromVoid more generic which gives better ergonomics #143

[RFC] Make the implementations of FromVoid more generic which gives better ergonomics #143

Conversation

jrmuizel commented Jan 14, 2018 • edited by larsbergstrom

nox commented Jan 14, 2018

jrmuizel commented Jan 14, 2018

jrmuizel commented Jan 14, 2018

nox commented Jan 14, 2018

faern commented Jan 15, 2018

This comment has been minimized.

faern Jan 15, 2018

This comment has been minimized.

faern Jan 15, 2018

This comment has been minimized.

jrmuizel Jan 15, 2018

This comment has been minimized.

nox Jan 15, 2018

This comment has been minimized.

faern Jan 15, 2018

This comment has been minimized.

jrmuizel Jan 15, 2018

faern commented Jan 15, 2018

jrmuizel commented Jan 15, 2018

bors-servo commented Jan 18, 2018

jrmuizel commented Jan 14, 2018 •

edited by larsbergstrom