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

kbtree: add kb_itr_prev and more precise kb_itr_getp #73

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bfredl
Copy link

@bfredl bfredl commented Aug 7, 2016

It doesn't look to me like kb_itr_get is working: for instance the iterator will always store index 0 for the root node no matter what is the correct index in the root node, and the index found in the innermost traversed node (either because the key was found or an external node was reached) is never stored.

This PR implements a working version (to my testing), called kb_itr_getp, which also has the following useful behavior when the key is not found: the iterator then cannot be directly dereferenced, but kb_itr_next and kb_itr_prev (also added with this PR) will still take the iterator to the closest next or previous existing value respectively.

Also just like itr_next and itr_valid this returns nonzero when the iterator is valid (the key was found), it looks like kb_itr_get intended opposite behavior for some reason.

A simple test program to check the intended behavior: https://gist.github.com/bfredl/211dade1f913171dc0c95b195840fdfe

@justinmk
Copy link

justinmk commented Aug 7, 2016

@bfredl
Copy link
Author

bfredl commented Aug 7, 2016

Not sure, that looks more like a benchmark than a unit test, though my test could be turned more benchmarklike by making sure time is not dominated by verification, either by qsort-ing the list or precomputing some random permutation of the odd numbers before starting the timing.

@bfredl bfredl mentioned this pull request Aug 7, 2016
31 tasks
@stranger48
Copy link

Hi bfredl,

there's kb_itr_first() in kbtree which works perfectly. Is there a way to implement function returning end() iterator of btree?

Thanks

@bfredl
Copy link
Author

bfredl commented May 12, 2017

That should be perfectly analogous to kb_itr_first, but always go down the last subnode instead of the first subnode at every level. Or use kb_itr_getp with a MAX_KEY value.

@stranger48
Copy link

thanks for a quick reponse and points. I'll try (although I'm not sure I fully understand at this stage how everything works).

@till-s
Copy link

till-s commented Apr 16, 2020

I created an issue #141 for this bug. This PR seems to fix the kb_itr_get() bug -- in principle, that is; the newly introduced kb_itr_getp() is correct but the original kb_itr_get() is not fixed by this PR. Note also that the return value is inversed, i.e.

static inline int kb_itr_get_##name(kbtree_##name##_t *b, const key_t  k, kbitr_t *itr)
{
    return ! kb_itr_getp_##name(b, &k, itr);
}

@till-s till-s mentioned this pull request Apr 16, 2020
if (itr->p->x && itr->p->i >= 0) return 1; \
} \
} \
static int kb_itr_getp_##name(kbtree_##name##_t *b, const key_t * __restrict k, kbitr_t *itr) \

Choose a reason for hiding this comment

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

How about adding inline here? The compiler will trigger defined but not used warnings when there is no inline.
The above definition has the same problem:

static int kb_itr_get_##name(

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.

5 participants