Make the Benchmark class public #2101
Conversation
|
I like this change a lot, it's always bothered me that we expose "internal". The window failures look like legitimate issues @jmr . |
jmr
force-pushed
the
benchmark-pub
branch
2 times, most recently
from
e145307 to
0e05338
Compare
|
this looks good, but will users of the library see broken builds if they're using |
Only with |
|
ok, this LGTM, but i'd like a second maintainer to take a look given it's an API change: @LebedevRI i think we'll need a release after this too. |
It's an ABI change, but the old API continues to work.
Why? Isn't |
|
only because there's been a few changes since the last release and this is relatively significant. also it's nicer to import internally point releases rather than commit SHAs. |
|
If i'm quite honest, this looks like a change just for the sake of change.
|
|
we should probably fix those too, in follow-ups. I agree with the author that we shouldn't have a public API that has the |
include/benchmark/benchmark.h
Outdated
| // Define alias of ThreadRunner factory function type | ||
| using threadrunner_factory = | ||
| std::function<std::unique_ptr<ThreadRunnerBase>(int)>; | ||
|
|
||
| typedef void(Function)(State&); |
There was a problem hiding this comment.
Missing typedefs for these
There was a problem hiding this comment.
Are these intended to be user-facing? Maybe they should go back to internal?
There was a problem hiding this comment.
I'm not sure about Function, FunctionBenchmark and such,
but that whole threadrunner_factory thing
is clearly meant to be externally-avaliable.
Move ::benchmark::internal::Benchmark to ::benchmark::Benchmark. It's a bit odd that the documented way to pass arguments to a benchmark is with `benchmark::internal::Benchmark`. Make this public instead. https://github.com/google/benchmark/blob/v1.9.4/docs/user_guide.md#passing-arguments https://raw.githubusercontent.com/google/benchmark/refs/tags/v1.9.4/docs/user_guide.md#:~:text=void%20CustomArguments(benchmark%3A%3A-,internal%3A%3ABenchmark,-*%20b)%20%7B%0A%20%20for%20(int Keep ::benchmark::internal::Benchmark as a deprecated forwarding alias. Uses of Benchmark in the internal namespace need to explicitly use ::benchmark::Benchmark to avoid the deprecation warning.
LebedevRI
left a comment
LebedevRI
left a comment
There was a problem hiding this comment.
I don't see the point, but sure.
The main point is that the internal Google benchmark doesn't use Secondarily, if internal doesn't mean anything, I don't know what to think. |
3 participants
Move ::benchmark::internal::Benchmark to ::benchmark::Benchmark.
It's a bit odd that the documented way to pass arguments to a benchmark is with
benchmark::internal::Benchmark. Make this public instead.https://github.com/google/benchmark/blob/v1.9.4/docs/user_guide.md#passing-arguments
https://raw.githubusercontent.com/google/benchmark/refs/tags/v1.9.4/docs/user_guide.md#:~:text=void%20CustomArguments(benchmark%3A%3A-,internal%3A%3ABenchmark,-*%20b)%20%7B%0A%20%20for%20(int
Keep ::benchmark::internal::Benchmark as a deprecated forwarding alias. Uses of Benchmark in the internal namespace need to explicitly use ::benchmark::Benchmark to avoid the deprecation warning.