[FIX] Fix const compatibility for the shape_iterator of the kmer_hash view. #1756
Conversation
|
It would be great to have a bug report for this, which also explains the rationale why this is missing etc. Also since you are now working on that but as part of which issue and which iteration? |
|
Well, I only discovered this while investigating the coverage miss in #1654 (which was caused by not properly using the iterator_test_template). The minimiser view has the same bug but cannot be fixed before kmer_hash is fixed because it depends on the kmer_hash. |
smehringer
requested review from
a team and
joergi-w
and removed request for
a team
There was a problem hiding this comment.
This PR introduces a template parameter to the comparison operators that fixes this.
I couldn't find the code for this... did you forget to commit something?
And please check the test again. I have tried hard to understand the iterator test template and probably I have missed it, but I couldn't find where you test the shape iterator and I was also expecting to see these lines somewhere that you claimed to fix in this PR:
EXPECT_TRUE(std::ranges::begin(this->test_range) == std::ranges::cbegin(this->test_range));
const_iterator_type it{std::ranges::begin(this->test_range)};| @@ -211,6 +211,9 @@ class kmer_hash_view<urng_t>::shape_iterator | |||
| //!\brief The sentinel type of the underlying range. | |||
| using sentinel_t = std::ranges::sentinel_t<rng_t>; | |||
|
|
|||
| template <typename urng2_t> | |||
| friend class shape_iterator; | |||
There was a problem hiding this comment.
I have 2 questions here:
- Why do we need the friend declaration, although
shape_iteratoris public? - Neither in the modified test nor in the iterator test template the
shape_iteratoris used. How do you actually test the new functionality?
There was a problem hiding this comment.
Good question :)
So below, I introduced a new constructor from a shape_iterator which is specialised over a different urng_t. This implies that the two shape_iterators do not have the same type (differnet underlying type).
In order to copy over the members, I need to access them on rhs, but they are declared private. That's why I have to befriend every shape_iterator regardless of its underlying urng_t type in order to be able to access the private members.
Regarding the tests I answered below. If anything is still not clear please ask again :)
There was a problem hiding this comment.
Ah thank you, I think I understand now: A class with different template arguments is considered a different class and therefore private and protected member access is prohibited.
Sorry! I did not update the description! I had the templates there but it did not work because
The iterator test template has a member that is |
smehringer
force-pushed
the
range_fix
branch
2 times, most recently
from
7ebe490
to
fc17d23
Compare
Codecov Report
@@ Coverage Diff @@
## master #1756 +/- ##
==========================================
+ Coverage 97.62% 97.64% +0.01%
==========================================
Files 239 239
Lines 9061 9112 +51
==========================================
+ Hits 8846 8897 +51
Misses 215 215
Continue to review full report at Codecov.
|
|
I rebased such that the individual commits can be merged as is afterwards. |
When introducing the iterator test template to the kmer_view, the following does not compile:
const_iterator_type it{std::ranges::begin(this->test_range)};This PR introduces a custom constructor from the non-const version that fixes both problems.
.