[m-rev.] for review: handle I/O errors when reporting statistics

[Julien Fischer]

Hi Zoltan,

On Mon, 5 Apr 2021, Zoltan Somogyi wrote:

> On Mon, 5 Apr 2021 03:38:39 +1000 (AEST), Julien Fischer <jfischer at opturion.com> wrote:
>> @@ -288,21 +238,17 @@ ML_report_standard_stats(io.MR_MercuryFileStruct stream)
>>       long real_time_at_prev_stat = real_time_at_last_stat;
>>       real_time_at_last_stat = System.DateTime.Now.Ticks;
>>
>> -    try {
>> -        io.mercury_print_string(stream, System.String.Format(
>> -            ""[User time: +{0:F2}s, {1:F2}s Real time: +{2:F2}s, {3:F2}s]\\n"",
>> -            (user_time_at_last_stat - user_time_at_prev_stat),
>> -            (user_time_at_last_stat - user_time_at_start),
>> -            ((real_time_at_last_stat - real_time_at_prev_stat)
>> -                / (double) System.TimeSpan.TicksPerSecond),
>> -            ((real_time_at_last_stat - real_time_at_start)
>> -                / (double) System.TimeSpan.TicksPerSecond)
>> -        ));
>> -        // XXX At this point there should be a whole bunch of memory usage
>> -        // statistics.
>> -    } catch (System.SystemException e) {
>> -        // XXX how should we handle I/O errors when printing statistics?
>> -    }
>> +    io.mercury_print_string(stream, System.String.Format(
>> +        ""[User time: +{0:F2}s, {1:F2}s Real time: +{2:F2}s, {3:F2}s]\\n"",
>> +        (user_time_at_last_stat - user_time_at_prev_stat),
>> +        (user_time_at_last_stat - user_time_at_start),
>> +        ((real_time_at_last_stat - real_time_at_prev_stat)
>> +            / (double) System.TimeSpan.TicksPerSecond),
>> +        ((real_time_at_last_stat - real_time_at_start)
>> +           / (double) System.TimeSpan.TicksPerSecond)
>> +    ));
>> +    // XXX At this point there should be a whole bunch of memory usage
>> +    // statistics.
>>   }
>
> This kind of change is much easier to review when the diff is done with diff -b.
> As this diff is formatted now, I am just *assuming* that the stuff whose indentation
> changed had no other relevant changes.

The only non-indentation related change was the shifting of some exception
handlers.

>> @@ -91,23 +93,35 @@ MR_report_standard_stats(FILE *fp, int *line_number)
>>       eng = MR_get_engine();
>>   #endif
>>
>> -    fprintf(fp, "[User time: +%.3fs, %.3fs,",
>> -        (MR_user_time_at_last_stat - user_time_at_prev_stat) / 1000.0,
>> -        (MR_user_time_at_last_stat - MR_user_time_at_start) / 1000.0
>> -    );
>> +    if (
>> +        fprintf(fp, "[User time: +%.3fs, %.3fs,",
>> +            (MR_user_time_at_last_stat - user_time_at_prev_stat) / 1000.0,
>> +            (MR_user_time_at_last_stat - MR_user_time_at_start) / 1000.0
>> +        ) < 0
>> +    ) {
>> +        return errno;
>> +    }
>
> Instead of putting calls to fprintf into the condition of an if-then-else,
> I would *much* rather you just assigned the return value from the fprintf
> to a local var, and then tested *that*, here and everywhere else.

Done.

> Other than that, the diff is fine, though I do note that this diff does
> not extend to checking for I/O errors when printing tabling statistics.

I know, the previous diff didn't address the line number updates for
that tabling statistics either; I'll do both as a separate change.

Thanks for the review.

Julien.