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

Last state iterator #109

Merged
merged 18 commits into from Sep 19, 2022
Merged

Last state iterator #109

merged 18 commits into from Sep 19, 2022

Conversation

cheme
Copy link
Collaborator

@cheme cheme commented Sep 14, 2022

This is a small refactor of iterator logic when switching backward/forward.
Mainly removea bit of redundant code and makes that we do not return same value when changing iteration direction.

Currently draft as I need to reread it and clippy it, but @Tpt it may be interesting to run your fuzzer on this branch.

@Tpt
Copy link
Contributor

Tpt commented Sep 15, 2022

Thank you! This change seems to fix the bugs I encountered yesterday with iteration.

@cheme
Copy link
Collaborator Author

cheme commented Sep 15, 2022

Nice, to hear. Currently cleaning this branch.

@cheme
Copy link
Collaborator Author

cheme commented Sep 15, 2022

This PR should be reviewable.

@i1i1 with this pr, I did change the way the iterator behave when switching direction.
on a set of key a, b, c. and with calls next, next, prev, prev
before we had a -> b -> b -> a
now we have a -> b -> a -> none
it ends up being easier to fuzz (same behavior as btreemap).

@cheme cheme marked this pull request as ready for review September 15, 2022 08:47
let is_leaf = btree.depth as usize + 1 == self.state.len();
if let Some(state) = self.state.last_mut() {
let next = match (direction, &state.0) {
(_, LastIndex::Descend(_)) => unreachable!("exit function clean it"),
Copy link
Member

@arkpar arkpar Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(_, LastIndex::Descend(_)) => unreachable!("exit function clean it"),
(_, LastIndex::Descend(_)) => unreachable!("Last state should be clean after calling self.exit"),

Is it always cleaned by self.exit though? From what I can follow, sometimes self.enter is called instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, and enter will put a non descend too, this assumption is a bit of a headache, I propose to replace it with 2b95bce

Copy link
Contributor

@Tpt Tpt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel knowledgeable enough to properly review this PR.

I just have some code style suggestions (but feel free to ignore it).

I have also fuzzed the code and it seems to work \o/.

src/db.rs Show resolved Hide resolved
src/btree/iter.rs Outdated Show resolved Hide resolved
src/btree/iter.rs Outdated Show resolved Hide resolved
src/btree/iter.rs Outdated Show resolved Hide resolved
@cheme
Copy link
Collaborator Author

cheme commented Sep 15, 2022

I just have some code style suggestions (but feel free to ignore it).

they are very welcome, will fix in a bit.

@cheme
Copy link
Collaborator Author

cheme commented Sep 15, 2022

@i1i1 if you got time to triple check this PR, review is very welcome (even if things get merge already I will create new PR).
At least please notice the change in iterator behavior (probably this will get published in semver major).

This was referenced Sep 16, 2022
@arkpar arkpar merged commit 2d50667 into paritytech:master Sep 19, 2022
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.

None yet

3 participants