Fix validity problem #96
Conversation
|
Thanks for the pull request, and welcome! The contain-rs team is excited to review your changes, and you should hear from @reem (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. |
|
Yikes. Ok so this is definitely an improvement but I think more needs to be done to make this rigorously correct. In particular it is very easy to accidentally assert that the K and V exist transiently, leading to technical but unlikely to matter UB. I believe the correct fix here is a more subtle and frankly very annoying, in the same vein as these fixes we did for BTreeMap and LinkedList. |
|
see #97 for hasty notes |
| @@ -171,7 +171,8 @@ impl<K, V, S> LinkedHashMap<K, V, S> { | |||
| if self.head.is_null() { | |||
| // allocate the guard node if not present | |||
| unsafe { | |||
| self.head = Box::into_raw(Box::new(mem::uninitialized())); | |||
| let node_layout = std::alloc::Layout::new::<Node<K, V>>(); | |||
| self.head = std::alloc::alloc(node_layout) as *mut Node<K, V>; | |||
There was a problem hiding this comment.
Note that alloc::alloc is "expected to be deprecated".
There was a problem hiding this comment.
yeah but I doubt they'll actually do that since it's strictly worse
There was a problem hiding this comment.
although Layout also blows so what do I know
This PR addresses two problems.
mem::uninitializedis going to be deprecated.boolorchar), creating Node withmem::uninitialized()has undefined behavior.