ronantakizawa/codereview-bench huggingface dataset:
Repository: google/benchmark
File: src/timers.cc (C++)

Code before:
}

namespace {

std::string DateTimeString(bool local) {
  typedef std::chrono::system_clock Clock;
  std::time_t now = Clock::to_time_t(Clock::now());
  const std::size_t kStorageSize = 128;
  char storage[kStorageSize];
  std::size_t written;

  if (local) {
    written = std::strftime(storage, sizeof(storage), "%FT%T%z", ::localtime(&now));
  } else {
    written = std::strftime(storage, sizeof(storage), "%FT%TZ", ::gmtime(&now));
  }
  CHECK(written < kStorageSize);
  ((void)written);  // prevent unused variable in optimized mode.
  return std::string(storage);
}

}  // end namespace

std::string LocalDateTimeString() { return DateTimeString(true); }

}  // end namespace benchmark


Review comment (suggestion):
to confirm: i'm ok with losing the reentrant version given how this is used (and given there's really no equivalent on windows anyway).

Code after:
}

std::string LocalDateTimeString() {
  // Write the local time in RFC3339 format yyyy-mm-ddTHH:MM:SS+/-HH:MM.
  typedef std::chrono::system_clock Clock;
  std::time_t now = Clock::to_time_t(Clock::now());
  const std::size_t kTzOffsetLen = 6;
  const std::size_t kTimestampLen = 19;

  std::size_t tz_len;
  std::size_t timestamp_len;
  long int offset_minutes;
  char tz_offset_sign = '+';
  char tz_offset[kTzOffsetLen + 1];
  char storage[kTimestampLen + kTzOffsetLen + 1];

#if defined(BENCHMARK_OS_WINDOWS)
  std::tm *timeinfo_p = ::localtime(&now);
#else
  std::tm timeinfo;
  std::tm *timeinfo_p = &timeinfo;
  ::localtime_r(&now, &timeinfo);
#endif

  tz_len = std::strftime(tz_offset, sizeof(tz_offset), "%z", timeinfo_p);

  if (tz_len < kTzOffsetLen && tz_len > 1) {
    // Timezone offset was written. strftime writes offset as +HHMM or -HHMM,
    // RFC3339 specifies an offset as +HH:MM or -HH:MM. To convert, we parse
    // the offset as an integer, then reprint it to a string.

    offset_minutes = ::strtol(tz_offset, NULL, 10);
    if (offset_minutes < 0) {
      offset_minutes *= -1;
      tz_offset_sign = '-';
    }
    tz_len = ::sprintf(tz_offset, "%c%02li:%02li", tz_offset_sign,
        offset_minutes / 100, offset_minutes % 100);
    CHECK(tz_len == 6);
    ((void)tz_len); // Prevent unused variable warning in optimized build.
  } else {
    // Unknown offset. RFC3339 specifies that unknown local offsets should be
    // written as UTC time with -00:00 timezone.
#if defined(BENCHMARK_OS_WINDOWS)
    // Potential race condition if another thread calls localtime or gmtime.
    timeinfo_p = ::gmtime(&now);
#else
    ::gmtime_r(&now, &timeinfo);
#endif

    strncpy(tz_offset, "-00:00", kTzOffsetLen + 1);
Repository: google/benchmark
File: include/benchmark/benchmark.h (C/C++)

Code before:
// Helper macro to create a main routine in a test that runs the benchmarks
#define BENCHMARK_MAIN()                                                \
  int main(int argc, char** argv) {                                     \
    ::benchmark::Initialize(&argc, argv);                               \
    if (::benchmark::ReportUnrecognizedArguments(argc, argv)) return 1; \
    ::benchmark::RunSpecifiedBenchmarks();                              \
  }                                                                     \
  int main(int, char**)

// ------------------------------------------------------
// Benchmark Reporters

namespace benchmark {

struct CPUInfo {
  struct CacheInfo {
    std::string type;
    int level;
    int size;
    int num_sharing;
  };

  int num_cpus;
  double cycles_per_second;
  std::vector<CacheInfo> caches;
  std::pair<bool /*validity*/, bool /*value*/> scaling_enabled;
  std::vector<double> load_avg;

  static const CPUInfo& Get();

 private:
  CPUInfo();
  BENCHMARK_DISALLOW_COPY_AND_ASSIGN(CPUInfo);
};

// Adding Struct for System Information
struct SystemInfo {
  std::string name;
  static const SystemInfo& Get();

 private:
  SystemInfo();
  BENCHMARK_DISALLOW_COPY_AND_ASSIGN(SystemInfo);
};

// BenchmarkName contains the components of the Benchmark's name
// which allows individual fields to be modified or cleared before
// building the final name using 'str()'.
struct BenchmarkName {
  std::string function_name;
  std::string args;

Review comment (suggestion):
i imagine an enum is more natural here. 

```c++
enum Scaling {
  UNKNOWN,
  ENABLED,
  DISABLED
};
```

Code after:
// Helper macro to create a main routine in a test that runs the benchmarks
#define BENCHMARK_MAIN()                                                \
  int main(int argc, char** argv) {                                     \
    ::benchmark::Initialize(&argc, argv);                               \
    if (::benchmark::ReportUnrecognizedArguments(argc, argv)) return 1; \
    ::benchmark::RunSpecifiedBenchmarks();                              \
  }                                                                     \
  int main(int, char**)

// ------------------------------------------------------
// Benchmark Reporters

namespace benchmark {

struct CPUInfo {
  struct CacheInfo {
    std::string type;
    int level;
    int size;
    int num_sharing;
  };

  enum Scaling {
    UNKNOWN,
    ENABLED,
    DISABLED
  };

  int num_cpus;
  double cycles_per_second;
  std::vector<CacheInfo> caches;
  Scaling scaling;
  std::vector<double> load_avg;

  static const CPUInfo& Get();

 private:
  CPUInfo();
  BENCHMARK_DISALLOW_COPY_AND_ASSIGN(CPUInfo);
};

// Adding Struct for System Information
struct SystemInfo {
  std::string name;
  static const SystemInfo& Get();

 private:
  SystemInfo();
  BENCHMARK_DISALLOW_COPY_AND_ASSIGN(SystemInfo);
};
Repository: google/benchmark
File: src/benchmark_runner.h (C/C++)

Code before:
DECLARE_int32(benchmark_repetitions);

DECLARE_bool(benchmark_report_aggregates_only);

DECLARE_bool(benchmark_display_aggregates_only);

DECLARE_string(benchmark_perf_counters);

namespace benchmark {

namespace internal {

extern MemoryManager* memory_manager;

struct RunResults {
  std::vector<BenchmarkReporter::Run> non_aggregates;
  std::vector<BenchmarkReporter::Run> aggregates_only;

  bool display_report_aggregates_only = false;
  bool file_report_aggregates_only = false;
};

class BenchmarkRunner {
 public:
  BenchmarkRunner(const benchmark::internal::BenchmarkInstance& b_,
                  std::vector<BenchmarkReporter::Run>* complexity_reports_);

  int get_num_repeats() const { return repeats_total; }

  bool has_repeats_remaining() const {
    return get_num_repeats() != num_repetitions_done;
  }

  void do_one_repetition();

  RunResults&& get_results();

 private:
  RunResults run_results;

  const benchmark::internal::BenchmarkInstance& b;
  std::vector<BenchmarkReporter::Run>& complexity_reports;

  const double min_time;
  const int repeats_total;
  const bool has_explicit_iteration_count;

  int num_repetitions_done = 0;

  std::vector<std::thread> pool;


Review comment (performance):
style guide has changed. `complexity_reports` is non-optional so should be a non-const reference (https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs).. you can change here or we can do it in another PR.

Code after:
DECLARE_int32(benchmark_repetitions);

DECLARE_bool(benchmark_report_aggregates_only);

DECLARE_bool(benchmark_display_aggregates_only);

DECLARE_string(benchmark_perf_counters);

namespace benchmark {

namespace internal {

extern MemoryManager* memory_manager;

struct RunResults {
  std::vector<BenchmarkReporter::Run> non_aggregates;
  std::vector<BenchmarkReporter::Run> aggregates_only;

  bool display_report_aggregates_only = false;
  bool file_report_aggregates_only = false;
};

class BenchmarkRunner {
 public:
  BenchmarkRunner(const benchmark::internal::BenchmarkInstance& b_,
                  BenchmarkReporter::PerFamilyRunReports* reports_for_family);

  int GetNumRepeats() const { return repeats; }

  bool HasRepeatsRemaining() const {
    return GetNumRepeats() != num_repetitions_done;
  }

  void DoOneRepetition();

  RunResults&& GetResults();

  BenchmarkReporter::PerFamilyRunReports* GetReportsForFamily() const {
    return reports_for_family;
  };

 private:
  RunResults run_results;

  const benchmark::internal::BenchmarkInstance& b;
  BenchmarkReporter::PerFamilyRunReports* reports_for_family;

  const double min_time;
  const int repeats;
  const bool has_explicit_iteration_count;
Repository: google/benchmark
File: include/benchmark/benchmark.h (C/C++)

Code before:
#endif

#include <stdint.h>

#include <algorithm>
#include <cassert>
#include <cstddef>
#include <iosfwd>
#include <map>
#include <set>
#include <string>
#include <utility>
#include <vector>

#if defined(BENCHMARK_HAS_CXX11)
#include <atomic>
#include <initializer_list>
#include <type_traits>
#include <utility>
#endif

#if defined(_MSC_VER)
#include <intrin.h>  // for _ReadWriteBarrier
#endif

#include "memory_manager.h"

#ifndef BENCHMARK_HAS_CXX11
#define BENCHMARK_DISALLOW_COPY_AND_ASSIGN(TypeName) \
  TypeName(const TypeName&);                         \
  TypeName& operator=(const TypeName&)
#else
#define BENCHMARK_DISALLOW_COPY_AND_ASSIGN(TypeName) \
  TypeName(const TypeName&) = delete;                \
  TypeName& operator=(const TypeName&) = delete
#endif

#ifdef BENCHMARK_HAS_CXX17
#define BENCHMARK_UNUSED [[maybe_unused]]
#elif defined(__GNUC__) || defined(__clang__)
#define BENCHMARK_UNUSED __attribute__((unused))
#else
#define BENCHMARK_UNUSED
#endif

#if defined(__GNUC__) || defined(__clang__)
#define BENCHMARK_ALWAYS_INLINE __attribute__((always_inline))
#define BENCHMARK_NOEXCEPT noexcept
#define BENCHMARK_NOEXCEPT_OP(x) noexcept(x)
#elif defined(_MSC_VER) && !defined(__clang__)
#define BENCHMARK_ALWAYS_INLINE __forceinline

Review comment (suggestion):
i don't think this will work when the package is installed. it would need to be `"benchmark/memory_manager"` assuming that it's installed to `$INCLUDE_DIR/benchmark`.

this is (one reason) why we've avoided splitting the header in the past. it gets too gnarly to manage it across local development and installation on multiple systems.

Code after:
#endif

#include <stdint.h>

#include <algorithm>
#include <cassert>
#include <cstddef>
#include <iosfwd>
#include <limits>
#include <map>
#include <set>
#include <string>
#include <utility>
#include <vector>

#if defined(BENCHMARK_HAS_CXX11)
#include <atomic>
#include <initializer_list>
#include <type_traits>
#include <utility>
#endif

#if defined(_MSC_VER)
#include <intrin.h>  // for _ReadWriteBarrier
#endif

#ifndef BENCHMARK_HAS_CXX11
#define BENCHMARK_DISALLOW_COPY_AND_ASSIGN(TypeName) \
  TypeName(const TypeName&);                         \
  TypeName& operator=(const TypeName&)
#else
#define BENCHMARK_DISALLOW_COPY_AND_ASSIGN(TypeName) \
  TypeName(const TypeName&) = delete;                \
  TypeName& operator=(const TypeName&) = delete
#endif

#ifdef BENCHMARK_HAS_CXX17
#define BENCHMARK_UNUSED [[maybe_unused]]
#elif defined(__GNUC__) || defined(__clang__)
#define BENCHMARK_UNUSED __attribute__((unused))
#else
#define BENCHMARK_UNUSED
#endif

#if defined(__GNUC__) || defined(__clang__)
#define BENCHMARK_ALWAYS_INLINE __attribute__((always_inline))
#define BENCHMARK_NOEXCEPT noexcept
#define BENCHMARK_NOEXCEPT_OP(x) noexcept(x)
#elif defined(_MSC_VER) && !defined(__clang__)
#define BENCHMARK_ALWAYS_INLINE __forceinline
#if _MSC_VER >= 1900
Repository: google/benchmark
File: include/benchmark/benchmark.h (C/C++)

Code before:
    size_t name_field_width;
    static const char* executable_name;
    Context();
  };

  struct Run {
    static const int64_t no_repetition_index = -1;
    enum RunType { RT_Iteration, RT_Aggregate };

    Run()
        : run_type(RT_Iteration),
          aggregate_unit(kTime),
          error_occurred(false),
          iterations(1),
          threads(1),
          time_unit(kNanosecond),
          real_accumulated_time(0),
          cpu_accumulated_time(0),
          max_heapbytes_used(0),
          complexity(oNone),
          complexity_lambda(),
          complexity_n(0),
          report_big_o(false),
          report_rms(false),
          counters(),
          memory_result(0),
          allocs_per_iter(0.0) {}

    std::string benchmark_name() const;
    BenchmarkName run_name;
    int64_t family_index;
    int64_t per_family_instance_index;
    RunType run_type;
    std::string aggregate_name;
    StatisticUnit aggregate_unit;
    std::string report_label;  // Empty if not set by benchmark.
    bool error_occurred;
    std::string error_message;

    IterationCount iterations;
    int64_t threads;
    int64_t repetition_index;
    int64_t repetitions;
    TimeUnit time_unit;
    double real_accumulated_time;
    double cpu_accumulated_time;

    // Return a value representing the real time per iteration in the unit
    // specified by 'time_unit'.
    // NOTE: If 'iterations' is zero the returned value represents the
    // accumulated time.

Review comment (bug):
initializing a pointer to `0` is a bit weird. `NULL` or `nullptr` if it's supported on old enough compilers.

Code after:
  struct Context {
    CPUInfo const& cpu_info;
    SystemInfo const& sys_info;
    // The number of chars in the longest benchmark name.
    size_t name_field_width;
    static const char* executable_name;
    Context();
  };

  struct Run {
    static const int64_t no_repetition_index = -1;
    enum RunType { RT_Iteration, RT_Aggregate };

    Run()
        : run_type(RT_Iteration),
          aggregate_unit(kTime),
          error_occurred(false),
          iterations(1),
          threads(1),
          time_unit(kNanosecond),
          real_accumulated_time(0),
          cpu_accumulated_time(0),
          max_heapbytes_used(0),
          complexity(oNone),
          complexity_lambda(),
          complexity_n(0),
          report_big_o(false),
          report_rms(false),
          counters(),
          memory_result(NULL),
          allocs_per_iter(0.0) {}

    std::string benchmark_name() const;
    BenchmarkName run_name;
    int64_t family_index;
    int64_t per_family_instance_index;
    RunType run_type;
    std::string aggregate_name;
    StatisticUnit aggregate_unit;
    std::string report_label;  // Empty if not set by benchmark.
    bool error_occurred;
    std::string error_message;

    IterationCount iterations;
    int64_t threads;
    int64_t repetition_index;
    int64_t repetitions;
    TimeUnit time_unit;
    double real_accumulated_time;
    double cpu_accumulated_time;
Repository: google/benchmark
File: src/perf_counters.cc (C++)

Code before:
  for (int fd : counter_ids_) {
    close(fd);
  }
}
#else   // defined HAVE_LIBPFM
const bool PerfCounters::kSupported = false;

bool PerfCounters::Initialize() { return false; }

PerfCounters PerfCounters::Create(
    const std::vector<std::string>& counter_names) {
  if (!counter_names.empty()) {
    GetErrorLogInstance() << "Performance counters not supported.";
  }
  return NoCounters();
}

PerfCounters::~PerfCounters() = default;
#endif  // defined HAVE_LIBPFM

PerfCountersMeasurement* PerfCountersMeasurement::Create(
    const std::vector<std::string>& counter_names) {
  static std::unique_ptr<PerfCountersMeasurement> perf_counters_measurement_ptr;
  static std::vector<std::string> cached_counter_names;

  if (perf_counters_measurement_ptr) {
    BM_CHECK(cached_counter_names == counter_names);
    return perf_counters_measurement_ptr.get();
  }

  PerfCounters perf_counters = PerfCounters::Create(counter_names);
  perf_counters_measurement_ptr = std::unique_ptr<PerfCountersMeasurement> (
        new PerfCountersMeasurement(std::move(perf_counters)));
  cached_counter_names = counter_names;
  return perf_counters_measurement_ptr.get();
}
}  // namespace internal
}  // namespace benchmark


Review comment (performance):
strictly speaking, this is not thread safe.  two threads could call this and the second would overwrite the perf counters pointer, leading to a memory leak.  it might be better to use a lambda as the initializer of the pointer.

Code after:
  for (int fd : counter_ids_) {
    close(fd);
  }
}
#else   // defined HAVE_LIBPFM
const bool PerfCounters::kSupported = false;

bool PerfCounters::Initialize() { return false; }

PerfCounters PerfCounters::Create(
    const std::vector<std::string>& counter_names) {
  if (!counter_names.empty()) {
    GetErrorLogInstance() << "Performance counters not supported.";
  }
  return NoCounters();
}

void PerfCounters::CloseCounters() const {}
#endif  // defined HAVE_LIBPFM

Mutex PerfCountersMeasurement::mutex_;
int PerfCountersMeasurement::ref_count_ = 0;
PerfCounters PerfCountersMeasurement::counters_ = PerfCounters::NoCounters();

PerfCountersMeasurement::PerfCountersMeasurement(
    const std::vector<std::string>& counter_names)
    : start_values_(counter_names.size()), end_values_(counter_names.size()) {
  MutexLock l(mutex_);
  if (ref_count_ == 0) {
    counters_ = PerfCounters::Create(counter_names);
  }
  // We chose to increment it even if `counters_` ends up invalid,
  // so that we don't keep trying to create, and also since the dtor
  // will decrement regardless of `counters_`'s validity
  ++ref_count_;

  BM_CHECK(!counters_.IsValid() || counters_.names() == counter_names);
}

PerfCountersMeasurement::~PerfCountersMeasurement() {
  MutexLock l(mutex_);
  --ref_count_;
  if (ref_count_ == 0) {
    counters_ = PerfCounters::NoCounters();
  }
}

PerfCounters& PerfCounters::operator=(PerfCounters&& other) noexcept {
  if (this != &other) {
    CloseCounters();
Repository: google/benchmark
File: src/perf_counters.h (C/C++)

Code before:
    valid_read_ &= counters_.Snapshot(&start_values_);
    ClobberMemory();
  }

  BENCHMARK_ALWAYS_INLINE bool Stop(
      std::vector<std::pair<std::string, double>>& measurements) {
    assert(IsValid());
    // Tell the compiler to not move instructions above/below where we take
    // the snapshot.
    ClobberMemory();
    valid_read_ &= counters_.Snapshot(&end_values_);
    ClobberMemory();

    for (size_t i = 0; i < counters_.names().size(); ++i) {
      double measurement = static_cast<double>(end_values_[i]) -
                           static_cast<double>(start_values_[i]);
      measurements.push_back({counters_.names()[i], measurement});
    }

    return valid_read_;
  }

 private:
  static Mutex mutex_;
  static int ref_count_;
  static PerfCounters counters_;
  bool valid_read_ = true;
  PerfCounterValues start_values_;
  PerfCounterValues end_values_;
};

BENCHMARK_UNUSED static bool perf_init_anchor = PerfCounters::Initialize();

}  // namespace internal
}  // namespace benchmark

#endif  // BENCHMARK_PERF_COUNTERS_H


Review comment (suggestion):
Oh, these are declared here, i was afraid that they have completely global scope.
But i think that means the GUARDED_BY should also be here, not at the definition.

Code after:
    valid_read_ &= counters_.Snapshot(&start_values_);
    ClobberMemory();
  }

  BENCHMARK_ALWAYS_INLINE bool Stop(
      std::vector<std::pair<std::string, double>>& measurements) {
    assert(IsValid());
    // Tell the compiler to not move instructions above/below where we take
    // the snapshot.
    ClobberMemory();
    valid_read_ &= counters_.Snapshot(&end_values_);
    ClobberMemory();

    for (size_t i = 0; i < counters_.names().size(); ++i) {
      double measurement = static_cast<double>(end_values_[i]) -
                           static_cast<double>(start_values_[i]);
      measurements.push_back({counters_.names()[i], measurement});
    }

    return valid_read_;
  }

 private:
  static Mutex mutex_;
  GUARDED_BY(mutex_) static int ref_count_;
  GUARDED_BY(mutex_) static PerfCounters counters_;
  bool valid_read_ = true;
  PerfCounterValues start_values_;
  PerfCounterValues end_values_;
};

BENCHMARK_UNUSED static bool perf_init_anchor = PerfCounters::Initialize();

}  // namespace internal
}  // namespace benchmark

#endif  // BENCHMARK_PERF_COUNTERS_H
Repository: google/benchmark
File: src/benchmark_register.cc (C++)

Code before:
  BM_CHECK(setup != nullptr);
  setup_ = setup;
  return this;
}

Benchmark* Benchmark::Teardown(void (*teardown)(const benchmark::State&)) {
  BM_CHECK(teardown != nullptr);
  teardown_ = teardown;
  return this;
}

Benchmark* Benchmark::RangeMultiplier(int multiplier) {
  BM_CHECK(multiplier > 1);
  range_multiplier_ = multiplier;
  return this;
}

Benchmark* Benchmark::MinTime(double t) {
  BM_CHECK(t > 0.0);
  BM_CHECK(iterations_ == 0);
  min_time_ = t;
  return this;
}

Benchmark* Benchmark::MinWarmUpTime(double t) {
  BM_CHECK(t > 0.0);
  BM_CHECK(iterations_ == 0);
  min_warmup_time_ = t;
  return this;
}

Benchmark* Benchmark::Iterations(IterationCount n) {
  BM_CHECK(n > 0);
  BM_CHECK(IsZero(min_time_));
  BM_CHECK(IsZero(min_warmup_time_));
  iterations_ = n;
  return this;
}

Benchmark* Benchmark::Repetitions(int n) {
  BM_CHECK(n > 0);
  repetitions_ = n;
  return this;
}

Benchmark* Benchmark::ReportAggregatesOnly(bool value) {
  aggregation_report_mode_ = value ? ARM_ReportAggregatesOnly : ARM_Default;
  return this;
}

Benchmark* Benchmark::DisplayAggregatesOnly(bool value) {

Review comment (suggestion):
>=, i think. given the default is 0.0, i can imagine a user might want to ensure one benchmark _isn't_ run with warmup.

Code after:
  BM_CHECK(setup != nullptr);
  setup_ = setup;
  return this;
}

Benchmark* Benchmark::Teardown(void (*teardown)(const benchmark::State&)) {
  BM_CHECK(teardown != nullptr);
  teardown_ = teardown;
  return this;
}

Benchmark* Benchmark::RangeMultiplier(int multiplier) {
  BM_CHECK(multiplier > 1);
  range_multiplier_ = multiplier;
  return this;
}

Benchmark* Benchmark::MinTime(double t) {
  BM_CHECK(t > 0.0);
  BM_CHECK(iterations_ == 0);
  min_time_ = t;
  return this;
}

Benchmark* Benchmark::MinWarmUpTime(double t) {
  BM_CHECK(t >= 0.0);
  BM_CHECK(iterations_ == 0);
  min_warmup_time_ = t;
  return this;
}

Benchmark* Benchmark::Iterations(IterationCount n) {
  BM_CHECK(n > 0);
  BM_CHECK(IsZero(min_time_));
  BM_CHECK(IsZero(min_warmup_time_));
  iterations_ = n;
  return this;
}

Benchmark* Benchmark::Repetitions(int n) {
  BM_CHECK(n > 0);
  repetitions_ = n;
  return this;
}

Benchmark* Benchmark::ReportAggregatesOnly(bool value) {
  aggregation_report_mode_ = value ? ARM_ReportAggregatesOnly : ARM_Default;
  return this;
}

Benchmark* Benchmark::DisplayAggregatesOnly(bool value) {