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

add map method #89

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

add map method #89

wants to merge 3 commits into from

Conversation

dermetfan
Copy link
Contributor

@dermetfan dermetfan commented Sep 15, 2019

This adds a map method to the Tree.

I used your Clone implementation as guidance and tried to stay as close to your style as I could.

Unfortunately I had to resort to an unsafe block to get ownership of nodes' values. The old node's value is zeroed. This should be safe because map takes ownership of the tree so the nodes and their values cannot be accessed anymore. Please correct me if I missed something.

@dermetfan dermetfan force-pushed the feature/map branch 2 times, most recently from 1a2ff39 to d8aa991 Compare September 15, 2019 16:04
@iwburns
Copy link
Owner

iwburns commented Sep 16, 2019

Thanks so much for the PR!

I used your Clone implementation as guidance [...]

I wish I could but I can't take credit for the Clone impl; I believe it was added recently by another contributor.

This looks good at first glance, but I'd like to really dig into this and understand what's going on before merging. However, I've got quite a few other things going on right now and I don't think I'll be able to get to this for a few weeks.

I definitely will get back to this once I get some free time though. Thanks again for the contribution!

@dermetfan dermetfan force-pushed the feature/map branch 2 times, most recently from be05d7c to a19f1f9 Compare September 19, 2019 15:32
It compiles on WebAssembly but fails to load in the browser.
@iwburns
Copy link
Owner

iwburns commented Oct 20, 2019

@dermetfan I'm so sorry it has taken me so long to get back around to this.

Nothing major is jumping out at me here so I'm almost ready to merge this and push up a v1.8.0, but I did want to ask a question first. Above, you said:

Unfortunately I had to resort to an unsafe block to get ownership of nodes' values. The old node's value is zeroed. This should be safe because map takes ownership of the tree [...]

I believe you are correct that this is safe, but I do wonder why you decided to have map take ownership of the tree instead of just a reference. Is there any reason for this in particular?

I'm thinking it might be more useful to have a map method that just takes a reference to the tree so that the calling context can decide whether to drop the old tree or keep it around. Do you see any drawbacks to that approach?

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