From deae1d940c46c1d0caca2a4b397c5cebb14cb84d Mon Sep 17 00:00:00 2001 From: Dominic Hamon Date: Mon, 13 Aug 2018 15:56:13 +0100 Subject: [PATCH] Make State constructor private. The State constructor should not be part of the public API. Adding a utility method to BenchmarkInstance allows us to avoid leaking the RunInThread method into the public API. --- include/benchmark/benchmark.h | 10 ++++------ src/benchmark.cc | 18 ++++++++---------- src/benchmark_api_internal.cc | 15 +++++++++++++++ src/benchmark_api_internal.h | 8 ++++++-- src/benchmark_register.cc | 8 ++++---- 5 files changed, 37 insertions(+), 22 deletions(-) create mode 100644 src/benchmark_api_internal.cc diff --git a/include/benchmark/benchmark.h b/include/benchmark/benchmark.h index 04fa202fdf..609dc863e3 100644 --- a/include/benchmark/benchmark.h +++ b/include/benchmark/benchmark.h @@ -430,6 +430,7 @@ struct Statistics { }; namespace internal { +struct BenchmarkInstance; class ThreadTimer; class ThreadManager; @@ -664,12 +665,11 @@ class State { // Number of threads concurrently executing the benchmark. const int threads; - // TODO(EricWF) make me private + private: State(size_t max_iters, const std::vector& ranges, int thread_i, int n_threads, internal::ThreadTimer* timer, internal::ThreadManager* manager); - private: void StartKeepRunning(); // Implementation of KeepRunning() and KeepRunningBatch(). // is_batch must be true unless n is 1. @@ -677,7 +677,8 @@ class State { void FinishKeepRunning(); internal::ThreadTimer* timer_; internal::ThreadManager* manager_; - BENCHMARK_DISALLOW_COPY_AND_ASSIGN(State); + + friend struct internal::BenchmarkInstance; }; inline BENCHMARK_ALWAYS_INLINE bool State::KeepRunning() { @@ -931,9 +932,6 @@ class Benchmark { virtual void Run(State& state) = 0; - // Used inside the benchmark implementation - struct Instance; - protected: explicit Benchmark(const char* name); Benchmark(Benchmark const&); diff --git a/src/benchmark.cc b/src/benchmark.cc index dcdd23f40e..ffa75889d5 100644 --- a/src/benchmark.cc +++ b/src/benchmark.cc @@ -126,7 +126,7 @@ void UseCharPointer(char const volatile*) {} namespace { BenchmarkReporter::Run CreateRunReport( - const benchmark::internal::Benchmark::Instance& b, + const benchmark::internal::BenchmarkInstance& b, const internal::ThreadManager::Result& results, size_t memory_iterations, const MemoryManager::Result& memory_result, double seconds) { // Create report about this benchmark run. @@ -169,12 +169,10 @@ BenchmarkReporter::Run CreateRunReport( // Execute one thread of benchmark b for the specified number of iterations. // Adds the stats collected for the thread into *total. -void RunInThread(const benchmark::internal::Benchmark::Instance* b, - size_t iters, int thread_id, - internal::ThreadManager* manager) { +void RunInThread(const BenchmarkInstance* b, size_t iters, int thread_id, + ThreadManager* manager) { internal::ThreadTimer timer; - State st(iters, b->arg, thread_id, b->threads, &timer, manager); - b->benchmark->Run(st); + State st = b->Run(iters, thread_id, &timer, manager); CHECK(st.iterations() >= st.max_iterations) << "Benchmark returned before State::KeepRunning() returned false!"; { @@ -199,7 +197,7 @@ struct RunResults { }; RunResults RunBenchmark( - const benchmark::internal::Benchmark::Instance& b, + const benchmark::internal::BenchmarkInstance& b, std::vector* complexity_reports) { RunResults run_results; @@ -437,7 +435,7 @@ void State::FinishKeepRunning() { namespace internal { namespace { -void RunBenchmarks(const std::vector& benchmarks, +void RunBenchmarks(const std::vector& benchmarks, BenchmarkReporter* display_reporter, BenchmarkReporter* file_reporter) { // Note the file_reporter can be null. @@ -447,7 +445,7 @@ void RunBenchmarks(const std::vector& benchmarks, bool has_repetitions = FLAGS_benchmark_repetitions > 1; size_t name_field_width = 10; size_t stat_field_width = 0; - for (const Benchmark::Instance& benchmark : benchmarks) { + for (const BenchmarkInstance& benchmark : benchmarks) { name_field_width = std::max(name_field_width, benchmark.name.size()); has_repetitions |= benchmark.repetitions > 1; @@ -595,7 +593,7 @@ size_t RunSpecifiedBenchmarks(BenchmarkReporter* display_reporter, file_reporter->SetErrorStream(&output_file); } - std::vector benchmarks; + std::vector benchmarks; if (!FindBenchmarksInternal(spec, &benchmarks, &Err)) return 0; if (benchmarks.empty()) { diff --git a/src/benchmark_api_internal.cc b/src/benchmark_api_internal.cc new file mode 100644 index 0000000000..8d3108363b --- /dev/null +++ b/src/benchmark_api_internal.cc @@ -0,0 +1,15 @@ +#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 diff --git a/src/benchmark_api_internal.h b/src/benchmark_api_internal.h index 0bf0005e84..fe492c7eb6 100644 --- a/src/benchmark_api_internal.h +++ b/src/benchmark_api_internal.h @@ -6,6 +6,7 @@ #include #include #include +#include #include #include @@ -13,7 +14,7 @@ namespace benchmark { namespace internal { // Information kept per benchmark we may want to run -struct Benchmark::Instance { +struct BenchmarkInstance { std::string name; Benchmark* benchmark; AggregationReportMode aggregation_report_mode; @@ -31,10 +32,13 @@ struct Benchmark::Instance { double min_time; size_t iterations; int threads; // Number of concurrent threads to us + + State Run(size_t iters, int thread_id, internal::ThreadTimer* timer, + internal::ThreadManager* manager) const; }; bool FindBenchmarksInternal(const std::string& re, - std::vector* benchmarks, + std::vector* benchmarks, std::ostream* Err); bool IsZero(double n); diff --git a/src/benchmark_register.cc b/src/benchmark_register.cc index ffe97a1416..a85a4b44d8 100644 --- a/src/benchmark_register.cc +++ b/src/benchmark_register.cc @@ -78,7 +78,7 @@ class BenchmarkFamilies { // Extract the list of benchmark instances that match the specified // regular expression. bool FindBenchmarks(std::string re, - std::vector* benchmarks, + std::vector* benchmarks, std::ostream* Err); private: @@ -107,7 +107,7 @@ void BenchmarkFamilies::ClearBenchmarks() { } bool BenchmarkFamilies::FindBenchmarks( - std::string spec, std::vector* benchmarks, + std::string spec, std::vector* benchmarks, std::ostream* ErrStream) { CHECK(ErrStream); auto& Err = *ErrStream; @@ -152,7 +152,7 @@ bool BenchmarkFamilies::FindBenchmarks( for (auto const& args : family->args_) { for (int num_threads : *thread_counts) { - Benchmark::Instance instance; + BenchmarkInstance instance; instance.name = family->name_; instance.benchmark = family.get(); instance.aggregation_report_mode = family->aggregation_report_mode_; @@ -225,7 +225,7 @@ Benchmark* RegisterBenchmarkInternal(Benchmark* bench) { // FIXME: This function is a hack so that benchmark.cc can access // `BenchmarkFamilies` bool FindBenchmarksInternal(const std::string& re, - std::vector* benchmarks, + std::vector* benchmarks, std::ostream* Err) { return BenchmarkFamilies::GetInstance()->FindBenchmarks(re, benchmarks, Err); }