Allow specifying number of iterations via --benchmark_min_time.

[oontvoo]

Make the flag accept two new suffixes:

• x: number of iterations
• s: minimum number of seconds.

This matches the internal benchmark API.

[oontvoo]

@dmah42 Let me know if we should rename the flag in this patch too? Thanks!

[dmah42]

aside from the broken build, this is also a breaking change. before it's merged we should do a release and then release right after it's merged with clear documentation about the behaviour change.

i haven't checked the code yet.. but does it also accept the old behaviour (ie if the "s" is missing")?

[oontvoo]

but does it also accept the old behaviour (ie if the "s" is missing")?

Yes - if suffixes are omitted, then we'd assume it's the number of seconds for backward compat (But we do issue a warning just to encourage users to use the new format)

[oontvoo]

adding tests - please hold off on the review. Will ping when ready. Thanks!

[oontvoo]

Hi @LebedevRI, any thought on this? Thanks!

[LebedevRI]

This makes it look like other suffixes (ms?) are supported

[oontvoo]

Yes, we could extend the suffixes support later - but for the first iteration, let's keep it simple. (And I'd like to get the interface in place first)

[oontvoo]

Hi, if there's no further objection, can we please get this rolling? Thanks!

[dmah42]

i'd like @LebedevRI to approve too

[LebedevRI]

This check is present in release builds, right?

[dmah42]

no, it acts like assert and is disabled in NDEBUG builds

[LebedevRI]

Consider this rubber-stamped.

[LebedevRI]

(also, what about warm-up option, should it also be affected?)

[oontvoo]

(also, what about warm-up option, should it also be affected?)

No, this will not affect the warm-up option.
(We could expand that flag, if it's useful? l)

[dmah42]

some valid linker errors: min_time_parse_gtest.cc:(.text+0x6ba): undefined reference to benchmark::internal::ParseBenchMinTime(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const&)'`

[oontvoo]

some valid linker errors: min_time_parse_gtest.cc:(.text+0x6ba): undefined reference to benchmark::internal::ParseBenchMinTime(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const&)'`

Hmm, any idea why the definition is not seen?

(The function is defined in benchmark_runner.cc - and moreover,t he same code built in other configurations .... what's difference about these "shared" runs? 🤔 )

[oontvoo]

some valid linker errors: min_time_parse_gtest.cc:(.text+0x6ba): undefined reference to benchmark::internal::ParseBenchMinTime(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const&)'`

Hmm, any idea why the definition is not seen?

(The function is defined in benchmark_runner.cc - and moreover,t he same code built in other configurations .... what's difference about these "shared" runs? thinking )

Ok - now the tests all PASSED in Debug builds but FAILED in Release builds (due to "failed to die").
I must be missing something here but the #ifndef NDEBUG should have prevented the tests from running in Release mode - why are they still running? (and hence failing?)

[oontvoo]

some valid linker errors: min_time_parse_gtest.cc:(.text+0x6ba): undefined reference to benchmark::internal::ParseBenchMinTime(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const&)'`

Hmm, any idea why the definition is not seen?
(The function is defined in benchmark_runner.cc - and moreover,t he same code built in other configurations .... what's difference about these "shared" runs? thinking )

Ok - now the tests all PASSED in Debug builds but FAILED in Release builds (due to "failed to die"). I must be missing something here but the #ifndef NDEBUG should have prevented the tests from running in Release mode - why are they still running? (and hence failing?)

Ah, found it! TEST_BENCHMARK_LIBRARY_HAS_NO_ASSERTIONS is the needed guard

[dmah42]

any more comments here before we rebase and squash and merge?

[LebedevRI]

Consider this rubber-stamped.

[dmah42]

wonderful. thank you.