Skip to content

Conversation

@SimonSapin
Copy link
Contributor

This allows Option<NodeId> to be no larger than usize, and saves 32 bytes of memory per node.

To allow IDs to be used directly as indexes, the Vec<Node<T>> storage has an uninitialized item at index 0. Safety-wise, this "infects" the entire crate: every access to this Vec needs to make sure to skip that first (non-)item.

This allows `Option<NodeId>` to be no larger than `usize`,
and saves 32 bytes of memory per node.

To allow IDs to be used directly as indexes,
the `Vec<Node<T>>` storage has an **uninitialized** item at index 0.
Safety-wise, this "infects" the entire crate:
every access to this `Vec` needs to make sure to skip that first (non-) item.
@SimonSapin
Copy link
Contributor Author

Safety-wise, this "infects" the entire crate

At first I started moving the Tree struct into a small module where the vec field would be private, so that this unsafety could be "more contained". But then I realize this module would need a non-trivial amount of abstraction in order to support the rest of the crate, particularly iterators. Still, at ~1k lines I feel that this crate is just above the size where this kind of pervasive unsafety is manageable, so maybe this abstraction would pay off. What do you think?

@SimonSapin
Copy link
Contributor Author

#16 was an alternative that offset IDs by 1 instead of having an uninitialized Vec item.

@causal-agent
Copy link
Collaborator

I'm not really happy with the amount of unsafe in this crate, but I'm not sure I'd be happier with creating more abstraction inside it either. At least for now, this looks fine. I'll reconsider if more changes need to be made in the future.

@causal-agent causal-agent merged commit 52169be into rust-scraper:master Jan 18, 2019
@SimonSapin SimonSapin deleted the nonzero branch January 18, 2019 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants