ClockEstimate: Correct Average on Abort

[ax3l]

Description

The clock estimator has a potential division by zero and the average is wrong on abort. Using iteration + 1 would be correct for an average and is consistent with the non-aborted result which is also sum / (max(i)+1).

Seen via coverity in a downstream project.

[codecov]

Codecov Report

Merging #1490 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1490      +/-   ##
==========================================
- Coverage   80.42%   80.39%   -0.03%     
==========================================
  Files         121      121              
  Lines        3421     3426       +5     
==========================================
+ Hits         2751     2754       +3     
- Misses        670      672       +2

[codecov]

Codecov Report

Merging #1490 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1490      +/-   ##
==========================================
- Coverage   80.42%   80.39%   -0.03%     
==========================================
  Files         121      121              
  Lines        3421     3426       +5     
==========================================
+ Hits         2751     2754       +3     
- Misses        670      672       +2

[JoeyGrajciar]

i think that you should increase the i only in case of i == 0.

[ax3l]

i think that you should increase the i only in case of i == 0.

No, the sum is already calculated at that point, should definitely be i+1 in all cases.

[JoeyGrajciar]

so you want to divide sum by 2 in case of i=1?

[ax3l]

the sum starts with zero and adds for i=0 the first delta. Divided by one (i+1) this is still delta.

in the second iteration, i=1, the sum is the first value plus the second delta. You also want to divide by i+1 (now two).

The last iteration is i=iterations-1. At the same point where you divide sum/iterations you could divide sum/(i+1) (if you were just before exiting the loop).

[JoeyGrajciar]

okay in that case i acknowledge my mistake :)

[hbina]

Is it possible to rename this PR? Division by Zero is a bit misleading because imo this will never happen. The actual problem (as correctly pointed out by the author) is that the mean/average is actually inaccurate.

Edit: Reading through the code again, I think there is actually no problem here. The mean calculation is already correct. Note that the iteration is using pre-increment.

Catch2/include/internal/catch_timer.cpp

Line 28 in 20dd08e

for( std::size_t i = 0; i < iterations; ++i ) {

Edit2:I am just stupid, ignore edit1.

[ax3l]

Hi @hbina, this is zero-indexed for-loop, with range for i from 0 to iterations-1.

// main.cpp
#include <cstddef>
#include <iostream>


int main()
{
    std::size_t iterations = 4u;

    for( std::size_t i = 0; i < iterations; ++i ) {
        std::cout << i << std::endl;
    }
    return 0;
}

Compile & run:

$ g++ main.cpp && ./a.out                                                                                                                                         
0
1
2
3

$

Here is how for-loops work in C++:

iteration_expression: any expression, which is executed after every iteration of the loop and before re-evaluating condition. Typically, this is the expression that increments the loop counter

[ax3l]

@horenmar just in case: this is a simple fix for a corner-case, if you might want to take a look :)

[horenmar]

Yeah, the estimate is wrong in case of early-abort, and veeeeeeery potentially can cause a division by zero (if your clock resolution is worse than 3 seconds, 🤷‍♂️ ).

There is no reason not to fix it though.