Add ArgName() and ArgNames() methods to name arguments/ranges. #305
Conversation
|
Changes Unknown when pulling 17e1c40 on mkurdej:arg-names into * on google:master*. |
|
✅ Build benchmark 486 completed (commit 04692ab0a4 by @mkurdej) |
| while (state.KeepRunning()) { | ||
| } | ||
| } | ||
| BENCHMARK(BM_arg_name)->Arg(3)->ArgName("first"); |
There was a problem hiding this comment.
what happens if i call Args({1, 3})->ArgName("first") or if I call ArgName directly on the benchmark without Args being set?
Would this be a cleaner api if Args took a map<string, int>
There was a problem hiding this comment.
Well, to preserve current behaviour, the output will be exactly as in the current version if you do not specify ArgName. If you have more arguments than names, than only the first will be named.
One can find it a possible source of incoherence, but that's similar to the behaviour of Args, imagine calling it as Args({1, 2, 3})->Args({1, 2})->Args({1,2,3,4,5}) - that's user's responsibility to define all the arguments its benchmark will need.
For your suggestion, I find it a bit "heavy" to set argument names each time, I would certainly not want to have to write this: Args{{"first", 1}, {"second", 3}}->Args{{"first", 2}, {"second", 5}} and so on. And it is disturbing that the same argument can be named differently each time (or isn't it?)
Anyway, your remarks made me think of a case that I didn't anticipate: the user might want to name only few arguments, e.g. ArgNames({"first", "", "third"}), my current PR will write BM_benchmark/first:1/:2/third:3 (note the semicolon before 2). I'll correct it ASAP.
|
Changes Unknown when pulling c1c01b2 on mkurdej:arg-names into * on google:master*. |
|
✅ Build benchmark 487 completed (commit 5cef4c6beb by @mkurdej) |
| } | ||
|
|
||
| Benchmark* Benchmark::ArgNames(const std::vector<std::string>& names) { | ||
| arg_names_ = names; |
There was a problem hiding this comment.
should this be called after Args? If we can add a constraint on that, and then check that names.size() == args.size(), i think it would be much clearer to users when they do something unexpected. what do you think?
There was a problem hiding this comment.
Oh, I see now that there are checks in Arg/Args/Range/Ranges/DenseRange that use ArgsCnt. Previously I thought that you could call Args with different number of elements each time.
I'll add these checks then, it seems to be the way to go.
There was a problem hiding this comment.
I've added the constraint for the size. I've also modified ArgsCnt, so that it could be possible to call Args and ArgNames in any order. If you see a simpler/more elegant way to do this, I'm all ears.
|
Changes Unknown when pulling cfee1a5 on mkurdej:arg-names into * on google:master*. |
|
Changes Unknown when pulling 3f23832 on mkurdej:arg-names into * on google:master*. |
|
✅ Build benchmark 488 completed (commit c0d40d1987 by @mkurdej) |
|
✅ Build benchmark 489 completed (commit cc8d4c6804 by @mkurdej) |
|
Thanks! |
This PR allows the user to name arguments/ranges.
Instead of showing only "benchmark/1" (when called with
Arg(1)), one can call the benchmark withArg(1)->ArgName("arg")and the displayed name will be "benchmark/arg:1".Readability purposes only.