Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace unsafe raw pointers to the node tree #151

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

matthunz
Copy link
Contributor

@matthunz matthunz commented Oct 26, 2024

The methods for Node currently seem unsafe, as a Node can outlive its tree field of *mut Slab<Node>. It is also possible to obtain mutable references to the same node.

  • Replaces the current unsafe method of storing raw pointers to the tree of each node:
    struct Node {
      tree: *mut Slab<Node>,
      ...
    }
    With a container handle struct safely immutably borrowing from both:
    struct Node { ... }
    
    struct Handle<'a> {
        node: &'a Node,
        tree: &'a Slab<Node>,
    }
  • Adds documentation for Node fields and methods
  • Refactors and removes some dead code

@matthunz matthunz changed the title Refactoring and documention Replace unsafe raw pointers to the node tree Oct 26, 2024
@matthunz matthunz marked this pull request as ready for review October 27, 2024 00:57
@nicoburns
Copy link
Collaborator

@matthunz This mostly all looks pretty good, but:

  • Can we split it into separate PRs: in particular, the documentation and zoom changes seem standalone and uncontroversial, so would be good to get those merged!
  • Can we call the type NodeHandle?

You might also want to consider making the type:

struct NodeHandle<'a> {
    doc: &'a Document,
    node: &'a Node,
}

Which would allow the type to implement more things. I think we should also look at adding a

struct NodeHandleMut<'a> {
    doc: &mut 'a Document,
    node_id: NodeId,
}

which would allow a lot of the methods from Document to moved to NodeHandleMut.

@matthunz
Copy link
Contributor Author

Thanks for the review!

  • Can we split it into separate PRs: in particular, the documentation and zoom changes seem standalone and uncontroversial, so would be good to get those merged!

Definitely! I think I got sucked into a bit of a rabbit hole here... I'll try to factor out the less controversial stuff

  • Can we call the type NodeHandle?

Ooo yeah I actually like that a lot better

You might also want to consider making the type:

struct NodeHandle<'a> {
    doc: &'a Document,
    node: &'a Node,
}

That sounds interesting too, it does sound like we'd have more flexibility to add things. I also really like the mutable handle for the same reasons.

Just to clarify on that todo task though unfortunately it looks like stylo only excepts pointer-sized element types (specifically usize-sized, which sounds like a provenance issue so maybe we could submit a PR there 🤔). I was thinking we could do impl TElement for &'a NodeHandle<'a> but that's tricky with Traverser... I think it's possible with stylo as-is but I'm a little stuck so far

@matthunz matthunz marked this pull request as draft October 27, 2024 20:26
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