refactor: Add full pair iterator and for_each_full_pair #1973
Conversation
Also include a unit test
tests/algorithms/pairIterator.cpp
Outdated
| TEST(PairIterTest, FullPairs) | ||
| { | ||
| int x, y, size = 100; | ||
| FullPairIterator it(100); | ||
| for (x = 0; x < size; ++x) | ||
| { | ||
| for (y = 0; y < size; ++y) | ||
| { | ||
| auto [i, j] = *it; | ||
| EXPECT_EQ(i, x); | ||
| EXPECT_EQ(j, y); | ||
| ++it; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I want to suggest here that we do a less "pathological" test of the new iterator. Specifically something like taking a full (i.e. not halved) Array2D of random ints and get the sum through both the dumb double x,y loop as well as via the new iterator.
There was a problem hiding this comment.
I had a whole argument for why we shouldn't do this, but now I think that you're right. My argument had been that using the sum test proves that every pair is visited, but doesn't prove that we visited them in the correct order. However, it then becomes a philospohical question of whether there is a correct order or if that should just be considered an implementation detail. I'm now leaning closer to making it an implementation detail, so none of the tests should depend on it.
Also add some comments so no one else wastes time trying to use the iterator
This adds a
FullPairIteratorclass and afor_each_full_pairalgorithm that examines each ordered pair (as opposed tofor_each_pair, which only examines unique unordered pairs).