benchmark icon indicating copy to clipboard operation
benchmark copied to clipboard

`State::name` returns a string copy

Open romintomasetti opened this issue 6 months ago • 5 comments

I am not sure why the following line returns a copy of the name string: https://github.com/google/benchmark/blob/7f727750846552fb507d778b2b125dd6d32061bf/include/benchmark/benchmark.h#L978

If there is no particular reason, I can make a PR to return by const auto&.

romintomasetti avatar Jun 13 '25 19:06 romintomasetti

Because until recently, that header was only allowed to use C++03.

LebedevRI avatar Jun 13 '25 19:06 LebedevRI

Hum. I'm not sure to understand why C++03 incurred such a restriction...

Nevertheless, do you confirm that it should now return by const auto& ?

romintomasetti avatar Jun 13 '25 19:06 romintomasetti

Hum. I'm not sure to understand why C++03 incurred such a restriction...

auto didn't exist until C++11, did it?

Nevertheless, do you confirm that it should now return by const auto& ?

I would guess that it should not return owning string, yes.

LebedevRI avatar Jun 13 '25 21:06 LebedevRI

auto didn't exist until C++11, did it?

Well, I'm still surprised that it was not written const std::string& at the time.

So the question is really if there is a reason to return the name member by value - I don't see any in this case, but I'm not really familiar with the peculiarities of micro-benchmarking so I might be missing something.

romintomasetti avatar Jun 13 '25 21:06 romintomasetti

there's no reason, but i suspect most modern compilers are optimising out any benefit from returning by reference vs value.

go/totw/101 is a good reference (ha!) for this where it depends on how the returned value is being used and stored as to what makes sense, as well as the underlying lifetime of the name and if it can become a dangling reference. tl;dr: you might create a copy where there (now) wouldn't be one and you might introduce lifetime issues where there currently aren't any.

tl;dr, tl;dr: it's probably not worth changing.

dmah42 avatar Jun 16 '25 08:06 dmah42