BTree: refactor clone, mostly for readability #89513
Conversation
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
☔ The latest upstream changes (presumably #89512) made this pull request unmergeable. Please resolve the merge conflicts. |
ssomers
force-pushed
the
btree_cleanup_clone
branch
from
d74b578 to
2758023
Compare
|
r? rust-lang/libs |
apiraino
added
the
T-libs
|
r? @rust-lang/libs EDIT: sorry, managed to miss that I'd already done this. |
There was a problem hiding this comment.
Hm. I am not convinced that this is an improvement -- it looks like we're creating a new abstraction but only using it inside the clone code. Is there an expectation that future work will be able to reuse that abstraction? The PR description seems to imply maybe, but I'm not sure I'm reading it right.
I'm worried that the abstraction doesn't seem like it's really abstracting any significant code (rather just providing one-line wrappers), so I'm not sure it pulls its weight yet at least. Maybe you can talk a little bit about the intent/future plans?
| unsafe { kv.drop_key_val() }; | ||
| cur_edge = next_edge; | ||
| } | ||
| } |
There was a problem hiding this comment.
It looks like this doesn't replicate the continue-dropping-if-we-see-a-panic code, presumably because the currently only user only drops BareTrees if we're already panicking. But that seems like a footgun should we start using it somewhere that does need to keep dropping.
There was a problem hiding this comment.
presumably because
Actually because I didn't realize it belongs there.
Mark-Simulacrum
added
S-waiting-on-author
Not any work that I have lying around or plan to do. |
Simplifies the
clonemember, in particular itsclone_subtreefunction, at the expense of having more code elsewhere, which might be reusable but wasn't reused up to now. The new destructor exploits the fact that the underlying navigation code spots the end of the tree itself, if we destroy the tree in a single direction. In theory, this performs whimsically better than before because it no longer keeps track of the number of elements already cloned, just in case the clone needs to be rolled back after a panic.