Make repeated seeks possible

[pd3]

Repeated seeks with implicitly created region list wouldn't initialize all internal structures to the original clean state, as demonstrated by the issue #1362, resolved by this commit

[daviesrob]

This fixes the reported seeking bug, but I'm worried that it might have odd interactions with the missed_reg_handler callback should anyone use it. Admittedly it doesn't seem to have been used since bcftools commit 8457b56 , but it would be good to ensure that it behaves correctly. As far as I can tell, it would be best to not call missed_reg_handler at all when using bcf_sr_seek()?

[pd3]

I just force-pushed a fix to that, now also missed_reg_handler should do its job when bcf_sr_seek() is used

[daviesrob]

I thought of that as a fix, but I'm not sure if it's enough as while it stops missed_reg_handler getting called on the current chromosome, it also gets called later on on the target of the seek. So I think a seek to a high-ish position could call it, depending on the ranges present?

[pd3]

You are right. After a seek it will now behave as if this was the first region at the chromosome and will trigger the callback. I don't know what the right behavior should be. Since it is not used anymore, maybe we just let be for now and wait until a real world use case comes?

[daviesrob]

I'd be inclined to just disable it for the bcf_sr_seek() case. The cleanest way might be to rename bcf_sr_regions_overlap(), make it static and add an extra parameter to disable the callbacks. Then make a new bcf_sr_regions_overlap() which calls the new function with callbacks enabled so the current API is unchanged.

[pd3]

Or just call assert from bcf_sr_seek() if the callback is set, with appropriate comment added in the code

[daviesrob]

No, calling assert() would not be appropriate.

[pd3]

No, calling assert() would not be appropriate.

I don't understand. The current code is broken, therefore keeping the current behavior should not be appropriate. Trying to use the code with missed_reg_handler will likely cause unexpected behavior and this is in the programming space, not user space. In my view assert and/or API documentation is the right place.

[daviesrob]

Compiling the library with NDEBUG defined will make any assert() calls go away. It should therefore only be used to detect bugs inside HTSlib itself, and is not a reliable mechanism to report API misuse.

[pd3]

Okay, I modified it using a wrapper around a static function.

[daviesrob]

Looks OK now, thanks.