deprecate seqan3::views::drop #2540
Conversation
marehr
requested review from
a team and
SGSSGene
and removed request for
a team
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/seqan/seqan3/FEoSGaSXFyh5cxnRC9hukUZenGuP |
There was a problem hiding this comment.
I think there are missing some adjustments:
include/seqan3/alignment/pairwise/alignment_algorithm.hpp line 446
include/seqan3/search/detail/unidirectional_search_algorithm.hpp line 226
include/seqan3/io/sam_file/format_sam.hpp line 1158
include/seqan3/io/sam_file/format_sam.hpp line 1171
Very nice catch. The interesting thing is that the library build completely through. I'm currently building seqan3 without the definition of seqan3::views::drop. That should show me potential left-overs :) |
In file included from ../../../seqan3/include/seqan3/search/fm_index/bi_fm_index.hpp:22,
from ../../../seqan3/test/unit/search/fm_index_cursor/bi_fm_index_cursor_collection_test.cpp:15:
../../../seqan3/include/seqan3/search/fm_index/bi_fm_index_cursor.hpp: In member function ‘void gtest_suite_bi_fm_index_cursor_collection_test_::serialisation<gtest_TypeParam_>::TestBody() [with gtest_TypeParam_ = seqan3::bi_fm_index_cursor<seqan3::bi_fm_index<seqan3::dna4, seqan3::collection> >]’:
../../../seqan3/include/seqan3/search/fm_index/bi_fm_index_cursor.hpp:627:24: error: ‘it.seqan3::bi_fm_index_cursor<seqan3::bi_fm_index<seqan3::dna4, seqan3::collection> >::_last_char’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
627 | sdsl_char_type c = _last_char;
| ^
../../../seqan3/include/seqan3/search/fm_index/bi_fm_index_cursor.hpp: In member function ‘void gtest_suite_bi_fm_index_cursor_collection_test_::serialisation<gtest_TypeParam_>::TestBody() [with gtest_TypeParam_ = seqan3::bi_fm_index_cursor<seqan3::bi_fm_index<seqan3::dna5, seqan3::collection> >]’:
../../../seqan3/include/seqan3/search/fm_index/bi_fm_index_cursor.hpp:627:24: error: ‘it.seqan3::bi_fm_index_cursor<seqan3::bi_fm_index<seqan3::dna5, seqan3::collection> >::_last_char’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
627 | sdsl_char_type c = _last_char;
| ^ |
Codecov Report
@@ Coverage Diff @@
## master #2540 +/- ##
==========================================
+ Coverage 98.22% 98.23% +0.01%
==========================================
Files 269 269
Lines 10535 10540 +5
==========================================
+ Hits 10348 10354 +6
+ Misses 187 186 -1
Continue to review full report at Codecov.
|
SGSSGene
requested review from
a team and
eseiler
and removed request for
a team
|
|
||
| if (end_pos < begin_pos) | ||
| throw std::invalid_argument{"end_pos argument to seqan3::views::slice must be >= the begin_pos argument."}; | ||
|
|
||
| // urange | drop | take | ||
| return views::take(views::drop(std::forward<urng_t>(urange), begin_pos), end_pos - begin_pos); | ||
| // SEQAN3_WORKAROUND_GCC_100139 == 1 if std::views::{take, drop} does not type reduce (e.g. keep in type |
There was a problem hiding this comment.
Can you explain this to me again?
I would have thought that I need to use views::type_reduce if the std::views did not type erase; but apparently it's the other way around?
There was a problem hiding this comment.
Yeah, this is a bit complicated xD
std::views::drop(range_t, 2) -> view_t has the following rules:
range_t = std::span, returnsview_t = std::spanrange_t = std::string_view, returnsview_t = std::string_viewrange_t = std::ranges::subrange, returnsview_t = std::ranges::subrange- otherwise:
view_t = std::ranges::drop_view<range_t>
Currently, gcc 10 and 11 always return view_t = std::ranges::drop_view<range_t>.
seqan3::views::type_reduce has some additional special rules:
range_tisview, returns view as is
- this line is problematic if we do
view | type_reduce, because- a
std::ranges::drop_view<range_t>will be passed through; - that means only
type_reduce | viewmakes sense - one could argue that this rule should be checked in a different order (at least that the
borrowed_rangeones have priority)
- a
range_t = std::string const &, returnsview_t = std::string_viewrange_t = std::ranges::contiguous_range && std::ranges::borrowed_range && std::ranges::sized_range, returnsview_t = std::spanrange_t = std::ranges::random_access_range && std::ranges::borrowed_range && std::ranges::sized_range, returnsview_t = std::ranges::subrange
Anyway, since only type_reduce | view makes sense right now, we need that std::views::drop stays in the same type, otherwise it will always wrap std::ranges::drop_view around any type.
Example:
view_t = range_t | seqan3::views::slice(0, 0);
| range_t | view_t if SEQAN3_WORKAROUND_GCC_100139 == 1 |
view_t if SEQAN3_WORKAROUND_GCC_100139 == 0 |
expected view_t |
|---|---|---|---|
| std::span | std::span (explicit) | std::span | type_reduce¹ | drop¹ | take¹ = std::span |
std::span |
| std::string_view | std::string_view (explicit) | std::string_view | type_reduce¹ | drop² | take² = std::string_view |
std::string_view |
| std::ranges::subrange | std::ranges::subrange (explicit) | std::ranges::subrange | type_reduce⁴ | drop³ | take³ = std::ranges::subrange |
std::ranges::subrange |
| std::string const & | std::string_view (explicit) | std::string const & | type_reduce² | drop² | take² = std::string_view |
std::string_view |
| std::ranges::contiguous_range && std::ranges::borrowed_range && std::ranges::sized_range | std::span (explicit) | range_t | type_reduce³ | drop¹ | take¹ = std::span |
std::span |
| std::ranges::random_access_range && std::ranges::borrowed_range && std::ranges::sized_range | std::ranges::subrange (explicit) | range_t | type_reduce⁴ | drop³ | take³ = std::ranges::subrange |
std::ranges::subrange |
Superscript numbers denote rules.
…seqan3::dna4, seqan3::collection> >::_last_char’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
Co-authored-by: Enrico Seiler <eseiler@users.noreply.github.com>
fixes seqan/product_backlog#325