Hi,
As a newcomer who is still learning the codebase, I attempted to review the v2 patch.
The behavior looks correct to me.
I considered some edge cases and they all seem to be handled properly:
- Edge case 1: c1_cursor is behind match_index
This can occur when firstcount1 advances past c1_cursor due to bubble-up.
It appears to be handled by the condition at L2318.
- Edge case 2: All singletons are promoted, leaving no singletons to evict
The condition `else if (firstcount1 < track_cnt)` at L2348 ensures that
when firstcount1 == track_cnt (empty singleton region), the new value
is simply skipped. This looks correct.
- Edge case 3: c1_cursor exceeds track array bounds
While Ilia's proposal would simplify this, it's currently handled by L2321.
Regarding Ilia's simplification proposal:
> if (was_count1 && j < firstcount1)
> firstcount1--;
> if (c1_cursor < firstcount1)
> c1_cursor = firstcount1;
The simplification looks appealing. However, I may be misunderstanding something -
should the logic perhaps be:
if (was_count1 && j <= firstcount1)
firstcount1++;
if (c1_cursor < firstcount1)
c1_cursor = firstcount1;
My reasoning:
- Without `<=`, the case where j == firstcount1 would leave firstcount1 pointing
to a value with count=2 (no longer a singleton).
- I believe `firstcount1--` should be `firstcount1++`, since when a singleton
is promoted to multiply-seen, the singleton region shrinks from the left,
meaning the boundary moves right (increases).
Please correct me if I'm missing something.
Regards,
Tatsuya Kawata
