handle fixture construction after static initialization: rev1 #512

lijinpei · 2018-01-07T04:10:07Z

a revision of #511 and #505

coveralls · 2018-01-07T04:21:44Z

Coverage increased (+0.01%) to 86.939% when pulling 0800b71 on lijinpei:v2-fixture into e1c3a83 on google:master.

AppVeyorBot · 2018-01-07T05:13:27Z

✅ Build benchmark 947 completed (commit b122cfb5ec by @lijinpei)

dmah42 · 2018-01-09T16:51:22Z

include/benchmark/benchmark.h

+namespace internal {
+// Take ownership of the pointer and register the benchmark. Return the
+// registered benchmark.
+Benchmark* RegisterBenchmarkInternal(Benchmark*);


why is this now under HAS_CXX11? I don't think it was before.

Because this is only needed if you use lambda benchmarks, function and fixture benchmarks are registered using the non-internal RegisterBenchmark().

dmah42 · 2018-01-09T16:54:30Z

src/benchmark_register.cc

  return static_cast<int>(args_.front().size());
 }

+void Benchmark::Init() {}


this should be in benchmark.cc, right?

All Benchmakr:: member functions are already in benchmark_register.cc, maybe we shall use a seperate commit to move them to benchmark.cc

huh. yeah, probably move benchmark member functions to benchmark.cc and the stuff there to benchmark_main.cc or something.

dmah42 · 2018-01-09T16:55:32Z

src/benchmark_register.cc

-void FunctionBenchmark::Run(State& st) { func_(st); }
+// The class used to hold all Benchmarks created from static function.
+// (ie those created using the BENCHMARK(...) macros.
+class FunctionBenchmark : public Benchmark {


i'd be quite happy if these could be move out to their own header/source files (in src/).

OK, maybe I shall also move benchmark's member function definitions to benchmark.cc?

dmah42 · 2018-01-09T16:55:56Z

src/benchmark_register.cc


+internal::Benchmark* RegisterBenchmark(const char* name, internal::Function* func) {
+  auto* p_benchmark = new internal::FunctionBenchmark(name, func);
+  internal::BenchmarkFamilies::GetInstance()->AddBenchmark(std::unique_ptr<internal::Benchmark>(p_benchmark));


line length should be <= 80

dmah42 · 2018-01-09T16:56:04Z

src/benchmark_register.cc

+
+Benchmark* RegisterBenchmark(const char *name, FixtureCreator* creator) {
+  auto* p_benchmark = new FixtureBenchmark(name, creator);
+  internal::BenchmarkFamilies::GetInstance()->AddBenchmark(std::unique_ptr<internal::Benchmark>(p_benchmark));


line length

dmah42 · 2018-01-09T16:57:02Z

test/fixture_init_test.cc

+  }
+}
+
+int main(int argc, char **argv) {


maybe you can use googletest for this instead of asserts.

OK, I will do it.

Of the existing 35 testcases, only one(statistics_test) uses gtest. I add two testcases which does essentially the same thing, fixture_init_test.cpp doesn't use gtest while fixture_init_gtest.cpp does.

is this file necessary any more?

gtest is a new addition so we're just starting to move testing over to it. you're ahead of the curve :)

lijinpei · 2018-01-10T03:51:53Z

src/benchmark_register.cc

+  Function* func_;
+};
+
+//=============================================================================//


just to mention it, all those comments are 81 characters width(not including the newline)

coveralls · 2018-01-10T04:30:32Z

Coverage increased (+0.01%) to 86.939% when pulling c02a75a on lijinpei:v2-fixture into e1c3a83 on google:master.

AppVeyorBot · 2018-01-10T04:40:35Z

❌ Build benchmark 950 failed (commit ca7b0cccaf by @lijinpei)

coveralls · 2018-01-10T04:45:10Z

Coverage decreased (-0.01%) to 86.916% when pulling 84e24f4 on lijinpei:v2-fixture into e1c3a83 on google:master.

AppVeyorBot · 2018-01-10T05:24:53Z

❌ Build benchmark 951 failed (commit 690b2a4035 by @lijinpei)

coveralls · 2018-01-10T18:43:47Z

Coverage decreased (-0.01%) to 86.916% when pulling 8bc7f4a on lijinpei:v2-fixture into e1c3a83 on google:master.

AppVeyorBot · 2018-01-10T18:48:41Z

✅ Build benchmark 952 completed (commit 529fc8788c by @lijinpei)

dmah42 · 2018-01-10T20:09:49Z

include/benchmark/benchmark.h

  virtual void BenchmarkCase(State&) = 0;
 };

+namespace internal {


can this be put inside one of the existing internal namespaces instead of adding another block? at some point i tried to clean these blocks up.. it'd be nice to not make it messier.

AppVeyorBot · 2018-01-11T04:46:56Z

❌ Build benchmark 953 failed (commit 7cd3f6b4a0 by @lijinpei)

coveralls · 2018-01-11T05:01:06Z

Coverage decreased (-0.2%) to 86.682% when pulling f0008ff on lijinpei:v2-fixture into e1c3a83 on google:master.

EricWF · 2018-01-11T22:40:18Z

@lijinpei When you want to update a PR, please use the same branch and PR on github -- don't create new ones every time.

You can either squash the old commits and force-push yourself, or you can simply push the new commits. When we merge the PR we'll squash the commits into a single one anyway.

EricWF · 2018-01-12T00:56:38Z

src/benchmarks.cc

+#include "benchmark_api_internal.h"
+
+namespace {
+using namespace benchmark;


I would just put the unnamed namespace inside benchmark::internal instead of adding using statements.

EricWF · 2018-01-12T00:57:13Z

test/fixture_init_gtest.cc

+int main(int argc, char **argv) {
+  ::testing::InitGoogleTest(&argc, argv);
+  benchmark::Initialize(&argc, argv);
+  return RUN_ALL_TESTS();


The test LGTM. Thanks.

AppVeyorBot · 2018-01-12T02:54:28Z

❌ Build benchmark 955 failed (commit d9940082d3 by @lijinpei)

AppVeyorBot · 2018-01-12T03:39:21Z

❌ Build benchmark 956 failed (commit a7d37aac99 by @lijinpei)

coveralls · 2018-01-12T04:58:26Z

Coverage decreased (-0.2%) to 86.682% when pulling df045ff on lijinpei:v2-fixture into 9f5694c on google:master.

lijinpei · 2018-01-18T14:15:13Z

I can revert to the version which doesn't change the header file very much if you don't want to merge the newest change.

dmah42 · 2018-01-18T16:25:47Z

it would be nice to keep the PR focused. so one PR to handle the issue here, and another to clean up the header would be most appreciated.

AppVeyorBot · 2018-01-18T18:40:56Z

❌ Build benchmark 962 failed (commit c7e2ac7617 by @lijinpei)

AppVeyorBot · 2018-01-18T18:44:28Z

❌ Build benchmark 963 failed (commit d64de82c6f by @lijinpei)

coveralls · 2018-01-20T03:10:41Z

Coverage decreased (-0.2%) to 86.741% when pulling f8ed087 on lijinpei:v2-fixture into 9f5694c on google:master.

coveralls · 2018-01-20T09:31:49Z

Coverage decreased (-0.01%) to 86.916% when pulling c88f257 on lijinpei:v2-fixture into 4fe0206 on google:master.

AppVeyorBot · 2018-01-21T16:17:02Z

❌ Build benchmark 966 failed (commit 0b6085ebd0 by @lijinpei)

AppVeyorBot · 2018-01-21T16:49:48Z

❌ Build benchmark 967 failed (commit 2b1d502e05 by @lijinpei)

dmah42 · 2018-05-29T12:20:43Z

Are you still looking in to this, @lijinpei ?

AppVeyorBot · 2018-06-20T16:25:54Z

❌ Build benchmark 1302 failed (commit bbce1f06c3 by @lijinpei)

lijinpei · 2018-06-22T02:37:21Z

I can re-start to work on this.
BTW, on my machine(gcc 8.1.1), state_assembly_test.cc compiles to the following state_assembly_test.s output, I think it is just a reorder of the loop header/body of what FileCheck means to check, but FileCheck won't accept it.

dmah42 · 2021-05-05T16:09:22Z

I'm going to close this as it appears abandoned. Please feel free to try again.

googlebot added the cla: yes label Jan 7, 2018

dmah42 reviewed Jan 9, 2018

View reviewed changes

lijinpei commented Jan 10, 2018

View reviewed changes

dmah42 reviewed Jan 10, 2018

View reviewed changes

EricWF reviewed Jan 12, 2018

View reviewed changes

lijinpei force-pushed the v2-fixture branch 2 times, most recently from 504356b to df045ff Compare January 12, 2018 02:16

lijinpei force-pushed the v2-fixture branch 2 times, most recently from f8ed087 to 05baa30 Compare January 18, 2018 18:33

lijinpei force-pushed the v2-fixture branch 2 times, most recently from 23836c6 to c88f257 Compare January 21, 2018 15:30

handle fixture construction after static initialization:rev3

c88f257

Merge remote-tracking branch 'google/master' into v2-fixture

a51fda2

dmah42 closed this May 5, 2021

handle fixture construction after static initialization: rev1 #512

handle fixture construction after static initialization: rev1 #512

Uh oh!

Conversation

lijinpei commented Jan 7, 2018

Uh oh!

coveralls commented Jan 7, 2018

Uh oh!

AppVeyorBot commented Jan 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jan 10, 2018

Uh oh!

AppVeyorBot commented Jan 10, 2018

Uh oh!

coveralls commented Jan 10, 2018

Uh oh!

AppVeyorBot commented Jan 10, 2018

Uh oh!

coveralls commented Jan 10, 2018

Uh oh!

AppVeyorBot commented Jan 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Jan 11, 2018

Uh oh!

coveralls commented Jan 11, 2018

Uh oh!

EricWF commented Jan 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Jan 12, 2018

Uh oh!

AppVeyorBot commented Jan 12, 2018

Uh oh!

coveralls commented Jan 12, 2018

Uh oh!

lijinpei commented Jan 18, 2018

Uh oh!

dmah42 commented Jan 18, 2018

Uh oh!

AppVeyorBot commented Jan 18, 2018

Uh oh!

AppVeyorBot commented Jan 18, 2018

Uh oh!

coveralls commented Jan 20, 2018 •

edited

Loading