tagvalue null pointer

[jblachly]

bam_aux_get may return nullpointer; ctor checks for this, but the other member functions (to, etc.) do not. they must check for null pointer (or will segfault), or else the ctor should allocate (probably a bad idea)

[jblachly]

currently there are assert(this.data !is null) in each member function (added after this issue raised) , but this is not adequate for production build that will have asserts removed , and variable user data (i.e. a different BAM file) could lead to segfault

[jblachly]

I will note that exists does include a runtime check and returns true/false. Obvoiusly we can recommend that library consumer check this first, but cannot enforce it

How do you feel about exceptions/errors ? ppl writing ultra high performance code tend to avoid , and other parts of our library and codebases are @nothrow

[charlesgregory]

My expectation was that the library user should make the check. In some cases a bam may always have a tag they are looking for. With that in mind we could avoid a branch from the check. Exceptions are nice for opening and closing files with dhtslib but I prefer performance where it can be gained. Though I have a more intimate understanding of some of htslib's inner workings than a user might. We could always make such checks debug only as with D the assumption is that you will test your code in debug mode and things such as array out of bounds checks are removed in production.

[jblachly]

After a lot of thought I want to reopen this. We don't have to fix right away, or even decide right away, but the appearance of a null-pointer which is dependent on specific user input (i.e., the bamfile) is very dangerous and may never be caught by a developer during testing.

As Rust has shown us, safety does not have to come with much or any speed penalty, and overall CPUs these days are excellent at branch prediction

What do you think about wrapping it in Nullable or something along those lines?

[charlesgregory]

A fair point, and a decent idea. Will have to find a nullable to use though. I think the dlang Nullable is rather new? I don't want to hold the library back by requiring a high dlang library version requirement.

[jblachly]

Also currently we don't have any external library deps -- only the stdlib -- that is a huge plus IMO.

That being said, there are several nullable implementations

phobos https://dlang.org/phobos/std_typecons.html
vibe.d https://vibed.org/api/taggedalgebraic.taggedalgebraic/TaggedAlgebraic
mir http://mir-core.libmir.org/mir_algebraic.html
optional https://code.dlang.org/packages/optional

[charlesgregory]

Commit 4f5a29c introduces enforce protections for most (all I think) functions with TagValue. This results in exceptions thrown if the data pointer is null or if you are trying to convert the data to the wrong type. We should still consider rewriting with an Optional. Though there is an argument to be made to wrap this into a larger github issue as there are several places where an Optional could be used in dhtslib. At the very least TagValue should be considerably safer.

[charlesgregory]

With the closing of #66 and the prior mentioned commit, TagValue should be considerably safer. I won't close this issue as I don't know if this is the best solution. But I am going to remove it from the v0.12.0 milestone so we can proceed with release.