Closed geeklint closed 2 years ago
Thanks for the PR. Sorry for the delay in getting to this. It all looks totally correct and legal (after I checked that pointers are allowed to be dangling). I can see why you did it this way, because you can save one usize per owner this way.
For maximum clarity in the code, though, I think it would be better to make the OwnerID a usize
, and keep an Option<Box<OwnerIdTarget>>
as well as an OwnerID
in the QCellOwner. This costs an extra usize
but means that the drop is taken care of. One of my priorities is for this code to be easy to review by people with not too much Rust knowledge, so that they trust it. Also, the OwnerID in the cell is not really a pointer. Rather it is an ID (that may have been derived from a pointer). So logically it really should be a usize.
Tell me what you think. Do you think it is worth keeping your optimisation to save one usize in the owner? Otherwise I think the code clarity is more important.
I can subscribe to code clarity being a higher priority than saving a few bytes. One thing to note however, adding a Box
field in QCellOwner
would have to be feature-gated on alloc
once no_std support is landed[1]. That just means some extra visual noise - I suppose it's a trade-off if you think the extra (internal) feature gates or the pointer semantics are more likely to be confusing to a code reviewer. My instinct is the latter is worse, and having a separate Box
field is a good idea, but I just figured I'd mention that it's not perfectly clear-cut.
I'll update the OwnerID to use usize now.
[1] I have a no_std PR basically prepared, I was just waiting to see how this one shook out.
I think having the ID as usize makes everything clearer, and is logically more correct. Having a feature gate on the box field makes perfect sense, and I don't think that is going to be a problem. Thanks
Great, thanks!
As discussed, this changes QCell from using a u32 ID type to a pointer type, and relies on the memory allocator to generate unique pointers.
I updated the qcell_ids test to remove the checks for reuse. Ironically, it actually still passed in debug mode on my machine, but it would rely on implementation details of the memory allocator.
If this gets merged, I'll close the other PR.