Add Ref to the sync module.

[wedsonaf]

No description provided.

[ojeda]

The usual quick review on the docs only.

As usual, it is a pleasure to review things when they are well-documented on the first go :-)

[jabedude]

@wedsonaf would it be a good idea to add sample usage of Ref to one of the example rust character devices?

[alex]

Can this just be NonNull::new(boxed.into_raw())?

[wedsonaf]

NonNull::new may fail (if its argument is null), in which case it returns None. NonNull::from OTOH never fails because its argument cannot be null -- by doing it the way I did I have one less failure path.

[alex]

Ok. I think this maybe makes more sense as mem::forget(boxed) then? We're not actually using the into_raw() ptr, which momentarily threw me.

[wedsonaf]

Now that I think about it, it is possible that some future version of Box::into_raw will return something else other than boxed.deref(), so I think it's better to use your original suggestion, even if it implies an extra failure path (as it should never happen, it's possible the optimiser remove it from the final binary).

[wedsonaf]

Oh, look at this: rust-lang/rust#47336

So the way to go is NonNull::from(Box::leak(boxed)) -- no failure path and still compatible with Box::from_raw!

[alex]

Since everything just accesses .count, would it make more sense to have this ->&AtomicUsize?

[wedsonaf]

I think an opaque RefCount is better for a couple of reasons:

1. We can easily add more fields in the future if we choose to. For example, we could have a debug mode where we link all the references so that we know at any given time what all the references are. We could also add some instrumentation/perf numbers like a high water mark for the number of references, etc.
2. In an earlier version of the code, RefCount didn't have any constructors: the only way to get one [outside the module] was through Ref::try_new, which took a closure as argument that had a RefCount as argument and returned T (which would then be wrapped in Ref). This was my attempt at enforcing that all instances of T be wrapped in Ref. It didn't really guarantee it because one could just 'exfiltrate' the RefCount instance and use it to build an unsafe T (it could never return because it wouldn't be able to construct the T that it needs to return, but one could exfiltrate T to another thread and sleep forever). This is a convoluted way to build an unsafe program, but it is a way nonetheless, so I had to make RefCounted an unsafe trait. Anyway, I still hope we'll find a way to implement this safely and make RefCounted safe; not allowing RefCount to be arbitrarily constructed may be part of the solution.

[wedsonaf]

@wedsonaf would it be a good idea to add sample usage of Ref to one of the example rust character devices?

Yes, it would. I will add one in my next PR as it allows file instances to share some state through their device registration. I'll have implementations of read and write there that use this.

(Everything I'm pushing is used in a driver I'm writing. I'm actually teasing apart the driver and generic pieces and trying to commit them here before I push the initial version of the driver. Anyway, the reason I mention this is that people will be able to see real usage then as well.)

[wedsonaf]

NonNull::new may fail (if its argument is null), in which case it returns None. NonNull::from OTOH never fails because its argument cannot be null -- by doing it the way I did I have one less failure path.

[wedsonaf]

I think an opaque RefCount is better for a couple of reasons:

1. We can easily add more fields in the future if we choose to. For example, we could have a debug mode where we link all the references so that we know at any given time what all the references are. We could also add some instrumentation/perf numbers like a high water mark for the number of references, etc.
2. In an earlier version of the code, RefCount didn't have any constructors: the only way to get one [outside the module] was through Ref::try_new, which took a closure as argument that had a RefCount as argument and returned T (which would then be wrapped in Ref). This was my attempt at enforcing that all instances of T be wrapped in Ref. It didn't really guarantee it because one could just 'exfiltrate' the RefCount instance and use it to build an unsafe T (it could never return because it wouldn't be able to construct the T that it needs to return, but one could exfiltrate T to another thread and sleep forever). This is a convoluted way to build an unsafe program, but it is a way nonetheless, so I had to make RefCounted an unsafe trait. Anyway, I still hope we'll find a way to implement this safely and make RefCounted safe; not allowing RefCount to be arbitrarily constructed may be part of the solution.

[fbq]

I think we need to add comments for every Ordering usage.

[alex]

One small comment, otherwise this LGTM.

[alex]

Ok. I think this maybe makes more sense as mem::forget(boxed) then? We're not actually using the into_raw() ptr, which momentarily threw me.