Hi! Thank you for the review and your fixes! See my answers below. > 3. I would rather say to '*an* existing'. Because there is no a one > certain setting to which all the iterators point after setting that > flag. Thank you, I agree. > 4. Even though that bsearch passes the tests, I failed to understand > it. Please, consider a more canonical bsearch on the branch in a > separate commit. Feel free to comment, if someone catches your attention > in my version and you don't agree. Your version is better, I agree, my code just implements lower/upper bound, but there is no point in it, because all settings are unique. Thanks! >> + if (low < count) { >> + int cmp = strcmp(name[low], key); >> + if ((cmp == 0 && is_including) || >> + (cmp > 0 && !is_eq)) { >> + *sid = low; >> + return 0; >> + } >> + } > > 5. This last strcmp can be avoided. Or at least > moved into the cycle. See my commit. > > All the same about 'reverse' version. I believe with your fixes it doesn’t really matter anymore. > > 6. What I don't like here is that we check is_set as many times, > as many modules we have. Moreover, if set_forward didn't find > anything, you fallback to a fullscan in that module, even though > it wont find anything with 100% guarantee. > > I fixed it, see my commit. The thing is that if set_forward fails, then sid = count and next_in_module will immediately return -1. But if we are going to remove modules, then this part will be rewritten anyway. > I fixed settings tests also - there was no a check that > real settings space, and a user defined one return the same tuple count. It > could happen, that one of them returns 0 tuples, and it would be considered > success. > > Additionally, I added a test, which tries all existing iterators on every > setting. Thanks, I agree with those.