id-tree icon indicating copy to clipboard operation
id-tree copied to clipboard

add map method

Open dermetfan opened this issue 5 years ago • 2 comments

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 avatar Sep 15 '19 15:09 dermetfan

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!

iwburns avatar Sep 16 '19 13:09 iwburns

@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?

iwburns avatar Oct 20 '19 21:10 iwburns