[NFC] BenchmarkRunner: always populate *_report_aggregates_only bools. (#708)

It is better to let the RunBenchmarks(), report() decide whether to actually *only* output aggregates or not, depending on whether there are actually aggregates. It's subtle indeed. Previously, `BenchmarkRunner()` always said that "if there are no repetitions, then you should never output only the repetitions". And the `report()` simply assumed that the `report_aggregates_only` bool it received makes sense, and simply used it. Now, the logic is the same, but the blame has shifted. `BenchmarkRunner()` always propagates what those benchmarks would have wanted to happen wrt the aggregates. And the `report()` lambda has to actually consider both the `report_aggregates_only` bool, and it's meaningfulness. To put it in the context of the patch series - if the repetition count was `1`, but `*_report_aggregates_only` was set to `true`, and we capture each iteration separately, then we will compute the aggregates, but then output everything, both the iteration, and aggregates, despite `*_report_aggregates_only` being set to `true`.

[NFC] BenchmarkRunner: always populate *_report_aggregates_only bools. (#708)
99d1356c · Roman Lebedev · GitHub · 9cacec8e · 99d1356c · 99d1356c
Unverified Commit 99d1356c authored Oct 18, 2018 by Roman Lebedev Committed by GitHub Oct 18, 2018
Hide whitespace changes
Inline Side-by-side

Showing with 11 additions and 14 deletions

benchmark.cc src/benchmark.cc +2 -2

benchmark_runner.cc src/benchmark_runner.cc +9 -12

No files found.
--- a/src/benchmark.cc
+++ b/src/benchmark.cc
@@ -267,8 +267,8 @@ void RunBenchmarks(const std::vector<BenchmarkInstance>& benchmarks,
      auto report = [&run_results](BenchmarkReporter* reporter,
                                   bool report_aggregates_only) {
        assert(reporter);
-        assert(
+        // If there are no aggregates, do output non-aggregates.
-            !(report_aggregates_only && run_results.aggregates_only.empty()));
+        report_aggregates_only &= !run_results.aggregates_only.empty();
        if (!report_aggregates_only)
          reporter->ReportRuns(run_results.non_aggregates);
        if (!run_results.aggregates_only.empty())

--- a/src/benchmark_runner.cc
+++ b/src/benchmark_runner.cc
@@ -136,20 +136,17 @@ class BenchmarkRunner {
        has_explicit_iteration_count(b.iterations != 0),
        pool(b.threads - 1),
        iters(has_explicit_iteration_count ? b.iterations : 1) {
-    if (repeats != 1) {
+    run_results.display_report_aggregates_only =
+        (FLAGS_benchmark_report_aggregates_only ||
+         FLAGS_benchmark_display_aggregates_only);
+    run_results.file_report_aggregates_only =
+        FLAGS_benchmark_report_aggregates_only;
+    if (b.aggregation_report_mode != internal::ARM_Unspecified) {
      run_results.display_report_aggregates_only =
-          (FLAGS_benchmark_report_aggregates_only ||
+          (b.aggregation_report_mode &
-           FLAGS_benchmark_display_aggregates_only);
+           internal::ARM_DisplayReportAggregatesOnly);
      run_results.file_report_aggregates_only =
-          FLAGS_benchmark_report_aggregates_only;
+          (b.aggregation_report_mode & internal::ARM_FileReportAggregatesOnly);
-      if (b.aggregation_report_mode != internal::ARM_Unspecified) {
-        run_results.display_report_aggregates_only =
-            (b.aggregation_report_mode &
-             internal::ARM_DisplayReportAggregatesOnly);
-        run_results.file_report_aggregates_only =
-            (b.aggregation_report_mode &
-             internal::ARM_FileReportAggregatesOnly);
-      }
    }
    for (int repetition_num = 0; repetition_num < repeats; repetition_num++) {