[m-rev.] for review: handle I/O errors when reporting statistics
Julien Fischer
jfischer at opturion.com
Mon Apr 5 16:51:35 AEST 2021
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.
More information about the reviews
mailing list