[FIX] not accepting BAM with empty header #2536
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/seqan/seqan3/4AdembVej9dTyC9fPALjAEcB4fSr |
eseiler
requested review from
a team and
MitraDarja
and removed request for
a team
MitraDarja
requested review from
a team and
marehr
and removed request for
a team
|
CI fails with: ../../../seqan3/include/seqan3/io/sam_file/format_bam.hpp:360:17: error: passing ‘const std::vector<std::__cxx11::basic_string<char> >’ as ‘this’ argument discards qualifiers [-fpermissive]
header.ref_ids().push_back(string_buffer);
^~~~~~ |
| string_buffer.resize(tmp32 - 1); | ||
| std::ranges::copy_n(std::ranges::begin(stream_view), tmp32 - 1, string_buffer.data()); // copy without \0 character | ||
| string_buffer.resize(l_name - 1); | ||
| std::ranges::copy_n(std::ranges::begin(stream_view), l_name - 1, string_buffer.data()); // copy without \0 character |
There was a problem hiding this comment.
In theory, this should copy the \0 character, because l_name is 1 + length including \0.
There was a problem hiding this comment.
Not sure what you mean :)
Do want to copy that \0 too?
There was a problem hiding this comment.
The comment says: We do not copy \0.
The code says: We copy \0.
So follow-up would be to double-check and, if we do not copy the \0, to add a comment as to why is that. Because l_name is one more than the length including \0, so l_name - 1 should include the \0.
| int32_t l_text{}; // length of header text including \0 character | ||
| int32_t n_ref{}; // number of reference sequences | ||
| int32_t l_name{}; // 1 + length of reference name including \0 character | ||
| int32_t l_ref{}; // length of reference sequence |
There was a problem hiding this comment.
Per specification, these should be uint32_t. Maybe there was a reason they are not, and maybe we can find that reason.
Just replacing does not work.
|
I'll do the follow-up issue once this is merged so that I can do permalinks to the code. |
Codecov Report
@@ Coverage Diff @@
## master #2536 +/- ##
=======================================
Coverage 98.22% 98.22%
=======================================
Files 269 269
Lines 10526 10535 +9
=======================================
+ Hits 10339 10348 +9
Misses 187 187
Continue to review full report at Codecov.
|
Resolves #1201