Zero len array #1674
Merged
Zero len array #1674
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This function was omitted from PR #1194. Zero-length 'B' arrays are valid, so remove this check too and add a test case.
Slices' optional tags follow the rules for BAM tagged fields, so zero-length 'B' arrays are valid. Slice tags are currently unused, so the question is somewhat moot and there are no test cases.
lbergelson
approved these changes
Aug 3, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR #1194 removed a number of Array.getLength(value) == 0 checks when setting B-array tagged fields, as such empty tagged field values had recently been made explicitly valid in SAM/BAM/CRAM files.
Unfortunately the existing such check in setUnsignedArrayAttribute() was omitted from that PR and has now caused the issue raised in samtools/hts-specs#728. Whatever additional problems might (but probably don't) exist elsewhere in the HTSJDK code for zero-length ML tags, this check for a generic zero-length array was incorrect and should be removed. A test case for setUnsignedArrayAttribute() has also been added.
In grepping for other candidates, I noticed one other potentially incorrect looking Array.getLength invocation.
This was in Slice::setAttribute(), which appears to set an optional tag in a Slice header. There are currently no such tags defined, so the question is somewhat moot but — as a separate commit for your consideration — I have also removed that check. (As there are no such tags currently defined, there are no test cases for these fields to which a test exercising this change could be added.)
--
Updated jmarshall pull request to remove unused Slice::setAttribute() methods.