Finer control of --regions vs --targets overlap

[pd3]

This is to address a long-standing design flaw in handling regions and targets,
as described in these BCFtools issues:
samtools/bcftools#1420
samtools/bcftools#1421

HTSlib (and BCFtools) recognize two sets of behaviors / options for resctricting VCF/BCF files by region, one
is for streaming (-t/-T) and one for index-gumping (-r/-R). They behave differently, the first includes
only records with POS coordinate within the regions, the other includes overlapping regions. This allows
to modify the default behavior and provides three options:

• Include only records with POS starting in the regions/targets
• Include VCF records that overlap regions/targets, even if POS itself is outside the regions
• Include only VCF records where the true variation overlaps regions/targets, e.g. consider the
  difference between TC>T- and C>-

Most importantly, this allows to make the regions and targets behave the same way.

Note that the default behavior remains unchanged.

[jmarshall]

@@ -110,7 +112,8 @@ typedef struct bcf_sr_regions_t
-    int is_bin;             // is open in binary mode (tabix access)
+    int is_bin:30,          // is open in binary mode (tabix access)
+        overlap:2;          // see BCF_SR_REGIONS_OVERLAP/BCF_SR_TARGETS_OVERLAP

This breaks the ABI on big-endian platforms. On all platforms, whether it's ABI breakage depends on implementation-defined behaviour; on x86_64 you probably get away with it (i.e., x.is_bin = 1 sets the same bits before and after this commit).

[jkbonfield]

We did briefly discuss this in our meeting too (or rather I asked the question what the impact on ABI was and the conclusion was it'd break, but we didn't get a plan of action).

It's possible we could detect big/little endian and switch the order at compile time (#ifdef HTS_BIG_ENDIAN), which helps, but as you say it's not guaranteed. I'm not so worried though about guarantees if we can at least validate on a whole bunch of different compilers and have assurances that practically speaking it's consistent.

There's also the issue that practically there's not much else we can do, unless we define it in code (is_bin becomes is_bin & BIN_MASK) which punts it from an ABI break to an API one. That's not really palatable either.

[pd3]

From a practical perspective, the is_bin is (should be) never used by user applications, so it is safe. Again, practically speaking. Undoubtedly one can think of an evil example.

[jkbonfield]

I know there are a lot of things in exposed structs which we label as internal simply because in C there's no distinction and we don't have the private component as we would we C++ classes. That's really a poor documentation thing though.

However I see it used twice within bcftools, so it's not an internal-to-htslib thing.

[jmarshall]

It is true that with bcf_sr_regions_init()/bcf_sr_regions_destroy() etc, bcf_sr_regions_t is probably mostly “effectively opaque for application code”, so this is mostly effectively safe enough. (It would be good if some of these considerations could be mentioned publicly on the PR or commit message.)

Those two instances of is_bin in bcftools are peeking at the similarly named htsFile field, which is explicitly forbidden in htslb/hts.h and invalidates the comment there that says only a really old version of bcftools looked directly at this field. I suppose these two instances were added more recently…

[pd3]

@jkbonfield It's true it's used in bcftools, but only once. And it should not be, @pd3 is a bad bad boy.