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

Code before:
// Copyright 2015 Google Inc. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//     http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#include "benchmark/benchmark.h"
#include "benchmark_api_internal.h"
#include "internal_macros.h"

#ifndef BENCHMARK_OS_WINDOWS
#ifndef __Fuchsia__
#include <sys/resource.h>
#endif
#include <sys/time.h>
#include <unistd.h>
#endif

#include <algorithm>
#include <atomic>
#include <condition_variable>
#include <cstdio>
#include <cstdlib>
#include <cstring>
#include <fstream>
#include <iostream>
#include <memory>
#include <thread>

#include "check.h"
#include "colorprint.h"
#include "commandlineflags.h"
#include "complexity.h"
#include "counter.h"
#include "internal_macros.h"
#include "log.h"
#include "mutex.h"

Review comment (suggestion):
can you define a BENCHMARK_OS_FUSCHIA alongside the BENCHMARK_OS_XXX macros elsewhere? that'll help be consistent and track any other per-OS changes.

Code after:
// Copyright 2015 Google Inc. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//     http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#include "benchmark/benchmark.h"
#include "benchmark_api_internal.h"
#include "internal_macros.h"

#ifndef BENCHMARK_OS_WINDOWS
#ifndef BENCHMARK_OS_FUCHSIA
#include <sys/resource.h>
#endif
#include <sys/time.h>
#include <unistd.h>
#endif

#include <algorithm>
#include <atomic>
#include <condition_variable>
#include <cstdio>
#include <cstdlib>
#include <cstring>
#include <fstream>
#include <iostream>
#include <memory>
#include <thread>

#include "check.h"
#include "colorprint.h"
#include "commandlineflags.h"
#include "complexity.h"
#include "counter.h"
#include "internal_macros.h"
#include "log.h"
#include "mutex.h"
Repository: google/benchmark
File: include/benchmark/benchmark.h (C/C++)

Code before:
  void PrintRunData(const Run& report);

  bool printed_header_;
  std::set<std::string> user_counter_names_;
};

// If a MemoryManager is registered, it can be used to report memory statistics
// for a run of the benchmark.
class MemoryManager {
 public:
  struct Result {
    explicit Result(int64_t iters)
        : num_allocs(0), max_bytes_used(0), iterations(iters) {}

    // The number of allocations made in total between Start and Stop.
    int64_t num_allocs;

    // The peak memory use between Start and Stop.
    int64_t max_bytes_used;

    // The number of iterations run for the memory test.
    const int64_t iterations;
  };

  // Implement this to start recording allocation information.
  virtual void Start() = 0;

  // Implement this to stop recording and fill out the given Result structure.
  virtual void Stop(Result* result) = 0;
};

inline const char* GetTimeUnitString(TimeUnit unit) {
  switch (unit) {
    case kMillisecond:
      return "ms";
    case kMicrosecond:
      return "us";
    case kNanosecond:
    default:
      return "ns";
  }
}

inline double GetTimeUnitMultiplier(TimeUnit unit) {
  switch (unit) {
    case kMillisecond:
      return 1e3;
    case kMicrosecond:
      return 1e6;
    case kNanosecond:
    default:

Review comment (suggestion):
`MemoryManager` has virtual functions but non-virtual destructor :)

Code after:
 public:
  JSONReporter() : first_report_(true) {}
  virtual bool ReportContext(const Context& context);
  virtual void ReportRuns(const std::vector<Run>& reports);
  virtual void Finalize();

 private:
  void PrintRunData(const Run& report);

  bool first_report_;
};

class BENCHMARK_DEPRECATED_MSG("The CSV Reporter will be removed in a future release")
      CSVReporter : public BenchmarkReporter {
 public:
  CSVReporter() : printed_header_(false) {}
  virtual bool ReportContext(const Context& context);
  virtual void ReportRuns(const std::vector<Run>& reports);

 private:
  void PrintRunData(const Run& report);

  bool printed_header_;
  std::set<std::string> user_counter_names_;
};

// If a MemoryManager is registered, it can be used to collect and report
// allocation metrics for a run of the benchmark.
class MemoryManager {
 public:
  struct Result {
    Result() : num_allocs(0), max_bytes_used(0) {}

    // The number of allocations made in total between Start and Stop.
    int64_t num_allocs;

    // The peak memory use between Start and Stop.
    int64_t max_bytes_used;
  };

  virtual ~MemoryManager() {}

  // Implement this to start recording allocation information.
  virtual void Start() = 0;

  // Implement this to stop recording and fill out the given Result structure.
  virtual void Stop(Result* result) = 0;
};

inline const char* GetTimeUnitString(TimeUnit unit) {
  switch (unit) {
Repository: google/benchmark
File: src/benchmark_api_internal.cc (C++)

Code before:
#include "benchmark_api_internal.h"

#include <memory>

namespace benchmark {
namespace internal {

std::unique_ptr<State> BenchmarkInstance::Run(
    size_t iters, int thread_id, internal::ThreadTimer* timer,
    internal::ThreadManager* manager) const {
  auto st = std::unique_ptr<State>(new State(iters, arg, thread_id, threads,
                                             timer, manager));
  benchmark->Run(*st);
  return st;
}

}  // internal
}  // benchmark


Review comment (suggestion):
Well, i should have commented here i guess.
Because it's a memory allocation.
I don't really expect this to matter in the *normal* usage, but *if* it can be avoided..

Code after:
#include "benchmark_api_internal.h"

namespace benchmark {
namespace internal {

State BenchmarkInstance::Run(
    size_t iters, int thread_id, internal::ThreadTimer* timer,
    internal::ThreadManager* manager) const {
  State st(iters, arg, thread_id, threads, timer, manager);
  benchmark->Run(st);
  return st;
}

}  // internal
}  // benchmark
Repository: google/benchmark
File: src/sysinfo.cc (C++)

Code before:
    res.push_back(C);
  }
  return res;
}
#endif

std::vector<CPUInfo::CacheInfo> GetCacheSizes() {
#ifdef BENCHMARK_OS_MACOSX
  return GetCacheSizesMacOSX();
#elif defined(BENCHMARK_OS_WINDOWS)
  return GetCacheSizesWindows();
#else
  return GetCacheSizesFromKVFS();
#endif
}

std::string GetSystemName() {
#if defined(BENCHMARK_OS_WINDOWS)
  std::string str;
  const unsigned COUNT = MAX_COMPUTERNAME_LENGTH+1;
  TCHAR  hostname[COUNT];
  DWORD DWCOUNT = COUNT;
  if (!GetComputerName(hostname, &DWCOUNT))
    return std::string("Unable to Get Host Name");
#ifndef UNICODE
  str = hostname;
#else
  std::wstring wStr = hostname;
  str = std::string(wStr.begin(), wStr.end());
#endif
  return str;
#else
#ifdef BENCHMARK_OS_MACOSX //Mac Doesnt have a copy for Host Name in it
#define HOST_NAME_MAX 64
#endif
  const unsigned COUNT = HOST_NAME_MAX;
  char hostname[COUNT];
  int retVal = gethostname(hostname, COUNT);
  if (retVal != 0) return std::string("Unable to Get Host Name");
  return std::string(hostname);
#endif
}

int GetNumCPUs() {
#ifdef BENCHMARK_HAS_SYSCTL
  int NumCPU = -1;
  if (GetSysctl("hw.ncpu", &NumCPU)) return NumCPU;
  fprintf(stderr, "Err: %s\n", strerror(errno));
  std::exit(EXIT_FAILURE);
#elif defined(BENCHMARK_OS_WINDOWS)
  SYSTEM_INFO sysinfo;

Review comment (suggestion):
Are you 100.0% sure this is safe?
https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-getcomputernamew
> On input, specifies the size of the buffer, in TCHARs. On output, the number of TCHARs copied to the destination buffer, not including the terminating null character.

I'd recommend to use 'returned' `DWCOUNT`.

Code after:
        break;
    }
    res.push_back(C);
  }
  return res;
}
#endif

std::vector<CPUInfo::CacheInfo> GetCacheSizes() {
#ifdef BENCHMARK_OS_MACOSX
  return GetCacheSizesMacOSX();
#elif defined(BENCHMARK_OS_WINDOWS)
  return GetCacheSizesWindows();
#else
  return GetCacheSizesFromKVFS();
#endif
}

std::string GetSystemName() {
#if defined(BENCHMARK_OS_WINDOWS)
  std::string str;
  const unsigned COUNT = MAX_COMPUTERNAME_LENGTH+1;
  TCHAR  hostname[COUNT] = {'\0'};
  DWORD DWCOUNT = COUNT;
  if (!GetComputerName(hostname, &DWCOUNT))
    return std::string("");
#ifndef UNICODE
  str = std::string(hostname, DWCOUNT);
#else
  //Using wstring_convert, Is deprecated in C++17
  using convert_type = std::codecvt_utf8<wchar_t>;
  std::wstring_convert<convert_type, wchar_t> converter;
  std::wstring wStr(hostname, DWCOUNT);
  str = converter.to_bytes(wStr);
#endif
  return str;
#else // defined(BENCHMARK_OS_WINDOWS)
#ifdef BENCHMARK_OS_MACOSX //Mac Doesnt have HOST_NAME_MAX defined
#define HOST_NAME_MAX 64
#endif
  char hostname[HOST_NAME_MAX];
  int retVal = gethostname(hostname, HOST_NAME_MAX);
  if (retVal != 0) return std::string("");
  return std::string(hostname);
#endif // Catch-all POSIX block.
}

int GetNumCPUs() {
#ifdef BENCHMARK_HAS_SYSCTL
  int NumCPU = -1;
  if (GetSysctl("hw.ncpu", &NumCPU)) return NumCPU;
Repository: google/benchmark
File: src/thread_timer.h (C/C++)

Code before:
#ifndef BENCHMARK_THREAD_TIMER_H
#define BENCHMARK_THREAD_TIMER_H

#include "check.h"
#include "timers.h"

namespace benchmark {
namespace internal {

class ThreadTimer {
 public:
  ThreadTimer(bool measure_process_cpu_time_)
      : measure_process_cpu_time(measure_process_cpu_time_) {}

  // Called by each thread
  void StartTimer() {
    running_ = true;
    start_real_time_ = ChronoClockNow();
    start_cpu_time_ = ReadCpuTimerOfChoice();
  }

  // Called by each thread
  void StopTimer() {
    CHECK(running_);
    running_ = false;
    real_time_used_ += ChronoClockNow() - start_real_time_;
    // Floating point error can result in the subtraction producing a negative
    // time. Guard against that.
    cpu_time_used_ +=
        std::max<double>(ReadCpuTimerOfChoice() - start_cpu_time_, 0);
  }

  // Called by each thread
  void SetIterationTime(double seconds) { manual_time_used_ += seconds; }

  bool running() const { return running_; }


Review comment (suggestion):
https://abseil.io/tips/94, which discusses the non-readability of callsites when using boolean parameters.

Consider two factory methods (`Create()` and `CreateProcessCpuTime()`) that can hide the boolean. The enum example in the above absl tip is likely overkill for this.

Code after:
#ifndef BENCHMARK_THREAD_TIMER_H
#define BENCHMARK_THREAD_TIMER_H

#include "check.h"
#include "timers.h"

namespace benchmark {
namespace internal {

class ThreadTimer {
  explicit ThreadTimer(bool measure_process_cpu_time_)
      : measure_process_cpu_time(measure_process_cpu_time_) {}

 public:
  static ThreadTimer Create() {
    return ThreadTimer(/*measure_process_cpu_time_=*/false);
  }
  static ThreadTimer CreateProcessCpuTime() {
    return ThreadTimer(/*measure_process_cpu_time_=*/true);
  }

  // Called by each thread
  void StartTimer() {
    running_ = true;
    start_real_time_ = ChronoClockNow();
    start_cpu_time_ = ReadCpuTimerOfChoice();
  }

  // Called by each thread
  void StopTimer() {
    CHECK(running_);
    running_ = false;
    real_time_used_ += ChronoClockNow() - start_real_time_;
    // Floating point error can result in the subtraction producing a negative
    // time. Guard against that.
    cpu_time_used_ +=
        std::max<double>(ReadCpuTimerOfChoice() - start_cpu_time_, 0);
Repository: google/benchmark
File: test/benchmark_gtest.cc (C++)

Code before:
#include <vector>

#include "../src/benchmark_register.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"

namespace {

using namespace benchmark::internal;

TEST(AddRangeTest, Simple) {
  std::vector<int> dst;
  AddRange(&dst, 1, 2, 2);
  EXPECT_THAT(dst, testing::ElementsAre(1, 2));
}

TEST(AddRangeTest, Simple64) {
  std::vector<int64_t> dst;
  AddRange(&dst, static_cast<int64_t>(1), static_cast<int64_t>(2), 2);
  EXPECT_THAT(dst, testing::ElementsAre(1, 2));
}

TEST(AddRangeTest, Advanced) {
  std::vector<int> dst;
  AddRange(&dst, 5, 15, 2);
  EXPECT_THAT(dst, testing::ElementsAre(5, 8, 15));
}

TEST(AddRangeTest, Advanced64) {
  std::vector<int64_t> dst;
  AddRange(&dst, static_cast<int64_t>(5), static_cast<int64_t>(15), 2);
  EXPECT_THAT(dst, testing::ElementsAre(5, 8, 15));
}


Review comment (suggestion):
these tests should likely be in namespace `benchmark::internal` which you can add above.

```c++
namespace benchmark {
namespace internal {
namespace {

...

}  // namespace
}  // namespace internal
}  // namespace benchmark
```

Code after:
#include <vector>

#include "../src/benchmark_register.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"

namespace benchmark {
namespace internal {
namespace {

TEST(AddRangeTest, Simple) {
  std::vector<int> dst;
  AddRange(&dst, 1, 2, 2);
  EXPECT_THAT(dst, testing::ElementsAre(1, 2));
}

TEST(AddRangeTest, Simple64) {
  std::vector<int64_t> dst;
  AddRange(&dst, static_cast<int64_t>(1), static_cast<int64_t>(2), 2);
  EXPECT_THAT(dst, testing::ElementsAre(1, 2));
}

TEST(AddRangeTest, Advanced) {
  std::vector<int> dst;
  AddRange(&dst, 5, 15, 2);
  EXPECT_THAT(dst, testing::ElementsAre(5, 8, 15));
}

TEST(AddRangeTest, Advanced64) {
  std::vector<int64_t> dst;
  AddRange(&dst, static_cast<int64_t>(5), static_cast<int64_t>(15), 2);
  EXPECT_THAT(dst, testing::ElementsAre(5, 8, 15));
}
Repository: google/benchmark
File: src/csv_reporter.cc (C++)

Code before:
#include "complexity.h"

#include <algorithm>
#include <cstdint>
#include <iostream>
#include <string>
#include <tuple>
#include <vector>

#include "check.h"
#include "string_util.h"
#include "timers.h"

// File format reference: http://edoceo.com/utilitas/csv-file-format.

namespace benchmark {

namespace {
std::vector<std::string> elements = {
    "name",           "iterations",       "real_time",        "cpu_time",
    "time_unit",      "bytes_per_second", "items_per_second", "label",
    "error_occurred", "error_message"};
}  // namespace

std::string CsvEscape(const std::string & s) {
  std::string tmp;
  for (char c : s) {
    switch (c) {
    case '"' : tmp += "\"\""; break;
    default  : tmp += c; break;
    }
  }
  return '"' + tmp + '"';
}

bool CSVReporter::ReportContext(const Context& context) {
  PrintBasicContext(&GetErrorStream(), context);
  return true;
}

void CSVReporter::ReportRuns(const std::vector<Run>& reports) {
  std::ostream& Out = GetOutputStream();

  if (!printed_header_) {
    // save the names of all the user counters
    for (const auto& run : reports) {
      for (const auto& cnt : run.counters) {
        if (cnt.first == "bytes_per_second" || cnt.first == "items_per_second")
          continue;
        user_counter_names_.insert(cnt.first);
      }

Review comment (suggestion):
```suggestion
  std::string tmp;
  tmp.reserve(s.size() + 2);
```


Code after:
#include "complexity.h"

#include <algorithm>
#include <cstdint>
#include <iostream>
#include <string>
#include <tuple>
#include <vector>

#include "check.h"
#include "string_util.h"
#include "timers.h"

// File format reference: http://edoceo.com/utilitas/csv-file-format.

namespace benchmark {

namespace {
std::vector<std::string> elements = {
    "name",           "iterations",       "real_time",        "cpu_time",
    "time_unit",      "bytes_per_second", "items_per_second", "label",
    "error_occurred", "error_message"};
}  // namespace

std::string CsvEscape(const std::string & s) {
  std::string tmp;
  tmp.reserve(s.size() + 2);
  for (char c : s) {
    switch (c) {
    case '"' : tmp += "\"\""; break;
    default  : tmp += c; break;
    }
  }
  return '"' + tmp + '"';
}

bool CSVReporter::ReportContext(const Context& context) {
  PrintBasicContext(&GetErrorStream(), context);
  return true;
}

void CSVReporter::ReportRuns(const std::vector<Run>& reports) {
  std::ostream& Out = GetOutputStream();

  if (!printed_header_) {
    // save the names of all the user counters
    for (const auto& run : reports) {
      for (const auto& cnt : run.counters) {
        if (cnt.first == "bytes_per_second" || cnt.first == "items_per_second")
          continue;
        user_counter_names_.insert(cnt.first);
Repository: google/benchmark
File: src/json_reporter.cc (C++)

Code before:
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#include "benchmark/benchmark.h"
#include "complexity.h"

#include <algorithm>
#include <cmath>
#include <cstdint>
#include <iomanip>  // for setprecision
#include <iostream>
#include <limits>
#include <string>
#include <tuple>
#include <vector>

#include "string_util.h"
#include "timers.h"

namespace benchmark {

namespace {

std::string StrEscape(const std::string & s) {
  std::string tmp;
  for (char c : s) {
    switch (c) {
    case '\b': tmp += "\\b"; break;
    case '\f': tmp += "\\f"; break;
    case '\n': tmp += "\\n"; break;
    case '\r': tmp += "\\r"; break;
    case '\t': tmp += "\\t"; break;
    case '\\': tmp += "\\\\"; break;
    case '"' : tmp += "\\\""; break;
    default  : tmp += c; break;
    }
  }
  return tmp;
}

std::string FormatKV(std::string const& key, std::string const& value) {
  return StrFormat("\"%s\": \"%s\"", StrEscape(key).c_str(), StrEscape(value).c_str());
}

std::string FormatKV(std::string const& key, const char* value) {
  return StrFormat("\"%s\": \"%s\"", StrEscape(key).c_str(), StrEscape(value).c_str());
}

std::string FormatKV(std::string const& key, bool value) {
  return StrFormat("\"%s\": %s", StrEscape(key).c_str(), value ? "true" : "false");

Review comment (suggestion):
```suggestion
  std::string tmp;
  tmp.reserve(s.size());
```


Code after:
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#include "benchmark/benchmark.h"
#include "complexity.h"

#include <algorithm>
#include <cmath>
#include <cstdint>
#include <iomanip>  // for setprecision
#include <iostream>
#include <limits>
#include <string>
#include <tuple>
#include <vector>

#include "string_util.h"
#include "timers.h"

namespace benchmark {

namespace {

std::string StrEscape(const std::string & s) {
  std::string tmp;
  tmp.reserve(s.size());
  for (char c : s) {
    switch (c) {
    case '\b': tmp += "\\b"; break;
    case '\f': tmp += "\\f"; break;
    case '\n': tmp += "\\n"; break;
    case '\r': tmp += "\\r"; break;
    case '\t': tmp += "\\t"; break;
    case '\\': tmp += "\\\\"; break;
    case '"' : tmp += "\\\""; break;
    default  : tmp += c; break;
    }
  }
  return tmp;
}

std::string FormatKV(std::string const& key, std::string const& value) {
  return StrFormat("\"%s\": \"%s\"", StrEscape(key).c_str(), StrEscape(value).c_str());
}

std::string FormatKV(std::string const& key, const char* value) {
  return StrFormat("\"%s\": \"%s\"", StrEscape(key).c_str(), StrEscape(value).c_str());
}

std::string FormatKV(std::string const& key, bool value) {
Repository: google/benchmark
File: src/thread_manager.h (C/C++)

Code before:
#ifndef BENCHMARK_THREAD_MANAGER_H
#define BENCHMARK_THREAD_MANAGER_H

#include <atomic>

#include "benchmark/benchmark.h"
#include "mutex.h"

namespace benchmark {
namespace internal {

class ThreadManager {
 public:
  ThreadManager(int num_threads)
      : alive_threads_(num_threads), start_stop_barrier_(num_threads) {}

  Mutex& GetBenchmarkMutex() const RETURN_CAPABILITY(benchmark_mutex_) {
    return benchmark_mutex_;
  }

  bool StartStopBarrier() EXCLUDES(end_cond_mutex_) {
    return start_stop_barrier_.wait();
  }

  void NotifyThreadComplete() EXCLUDES(end_cond_mutex_) {
    start_stop_barrier_.removeThread();
    if (--alive_threads_ == 0) {
      MutexLock lock(end_cond_mutex_);
      end_condition_.notify_all();
    }
  }

  void WaitForAllThreads() EXCLUDES(end_cond_mutex_) {
    MutexLock lock(end_cond_mutex_);
    end_condition_.wait(lock.native_handle(),
                        [this]() { return alive_threads_ == 0; });
  }

 public:

Review comment (suggestion):
I disagree with this change. A `ThreadManager` shouldn't be implicitly construct from an int.

Code after:
#ifndef BENCHMARK_THREAD_MANAGER_H
#define BENCHMARK_THREAD_MANAGER_H

#include <atomic>

#include "benchmark/benchmark.h"
#include "mutex.h"

namespace benchmark {
namespace internal {

class ThreadManager {
 public:
  explicit ThreadManager(int num_threads)
      : alive_threads_(num_threads), start_stop_barrier_(num_threads) {}

  Mutex& GetBenchmarkMutex() const RETURN_CAPABILITY(benchmark_mutex_) {
    return benchmark_mutex_;
  }

  bool StartStopBarrier() EXCLUDES(end_cond_mutex_) {
    return start_stop_barrier_.wait();
  }

  void NotifyThreadComplete() EXCLUDES(end_cond_mutex_) {
    start_stop_barrier_.removeThread();
    if (--alive_threads_ == 0) {
      MutexLock lock(end_cond_mutex_);
      end_condition_.notify_all();
    }
  }

  void WaitForAllThreads() EXCLUDES(end_cond_mutex_) {
    MutexLock lock(end_cond_mutex_);
    end_condition_.wait(lock.native_handle(),
                        [this]() { return alive_threads_ == 0; });
  }

 public: