Make a device struct in charge of its own memory #8
Conversation
|
I updated the diff to re-implement |
src/device.rs
Outdated
|
|
||
| _marker: PhantomData, | ||
| } | ||
| pub unsafe fn new<'b>(ptr: *const hid_device_info) -> Device { |
There was a problem hiding this comment.
The lifetime annotation here is not needed anymore.
|
I just realized I'm very stupid and never actually sent the review, and then entirely forgot about this. |
|
Since you got rustfmt enabled in your editor, could you make a first commit that runs So it's easier to review, and then everyone is happy stuff is rustfmt'd 🐼 |
|
Sorry for the long turn-around on this, but I ran cargo-fmt in #10 |
|
Could you rebase this please? |
|
rebased, and the diff is looking much better |
| } | ||
| impl Drop for Devices { | ||
| fn drop(&mut self) { | ||
| if !self.cur.is_null() { |
There was a problem hiding this comment.
This check should not be necessary as C free functions deal just fine with receiving NULL as their input, both in general and in particular hid_free_enumeration.
| _marker: PhantomData, | ||
| } | ||
| pub unsafe fn new(ptr: *const hid_device_info) -> Device { | ||
| Device { ptr } |
There was a problem hiding this comment.
Since each Device is taking ownership of its node in the linked list, we should probably get rid of the value in the next pointer. Even if we don't use it when dropping the struct, my C senses are tingling.
Though GH won't let me comment on the relevant piece of code as it's context, the next function in the iterator should treat the next pointer as a Option<const * _> and do something equivalent to Option::take in order to dereference it and get the next node. This is also what you would/should do when doing the equivalent in C as we are removing the nodes from the list.
All this to say I think in next() the code should be zeroing out the next pointer before passing it to this constructor.
Currently, the lifetime of a
Deviceis dependent on theDevicesiterator that created it. I don't see a real necessity for this, other than the simplicity of implementingDropgiven thehid_free_enumerationfunction. If we break from this function and write our ownDropthatfrees all of the fields manually, eachDeviceis wholly independent, and can be owned anywhere in memory. In my use case, I need to pass aDevicebetween threads, and this is not possible given that theDevicesshare the same lifetime.Also, the
hidapimaintainers have stated that the linux and Mac versions are completely thread safe, so it should be safe to markDeviceasSendon those platforms.