[amoralpha]

Fixes #410.

complexity_n now defaults to -1 (“not set”) instead of 0, so SetComplexityN(0) is distinguishable from never calling it, as suggested in the issue. The check in ComputeBigO is relaxed to >= 0. Since n == 0 is now reachable, I added guards for log2(0) in oLogN/oNLogN and for a zero denominator in MinimalLeastSq.

Tests: zero-starting ranges for oN, auto, lambda, and oLogN with Range(0, 1), plus a death test for the missing SetComplexityN case. Without the fix, complexity_benchmark fails with the exact error from the issue (“Did you forget to call SetComplexityN?”).

Design note: n == 0 carries no asymptotic information, so the motivation is only that Range(0, ...) is a natural parameterization and currently aborts with a misleading message. If you’d prefer, I can instead keep n == 0 runs in the report but exclude them from the BigO fit.

[LebedevRI]

The more i stare at this -1 the more i don't like it.
Please just do std::Optional<ComplexityN> complexity_n = None;

[LebedevRI]

Please update all complexity_n/complexity_n_ fields to all be Optional<>.
There should not be any == -1/<0/>=0 checks.

[amoralpha]

Please update all complexity_n/complexity_n_ fields to all be Optional<>. There should not be any == -1/<0/>=0 checks.

Thanks for the review. I ran into an issue making everything std::optional, so I’d like to ask for your input before going further.

ThreadManager::Result is internal, so std::optional works fine there. The problem is State::complexity_n_ and BenchmarkReporter::Run::complexity_n, which are in public headers. cxx11_test builds those headers in C++11/14 mode, where doesn’t exist, so it breaks (MSVC: 'optional': is not a member of 'std'). Everything else builds fine.

Also, changing the type of the public Run::complexity_n would break any custom reporter that reads it as an integer.

I see three options:

1.Bump cxx11_test to require C++17, and use std::optional everywhere, including the public headers.
2.Keep the public headers C++11-compatible: use std::optional in the internal implementation, and in the public structs use a plain value plus a bool flag.
3.Add a benchmark::Optional that is the same for all standards (no #if) and use it for every complexity_n.

Since this is more of a policy decision, I’d rather not pick one myself. Which would you prefer?

[LebedevRI]

Please update all complexity_n/complexity_n_ fields to all be Optional<>. There should not be any == -1/<0/>=0 checks.

Thanks for the review. I ran into an issue making everything std::optional, so I’d like to ask for your input before going further.

ThreadManager::Result is internal, so std::optional works fine there. The problem is State::complexity_n_ and BenchmarkReporter::Run::complexity_n, which are in public headers. cxx11_test builds those headers in C++11/14 mode, where doesn’t exist, so it breaks (MSVC: 'optional': is not a member of 'std'). Everything else builds fine.

Oh well, i forgot about that.

Also, changing the type of the public Run::complexity_n would break any custom reporter that reads it as an integer.

So here's the thing, this diff already breaks that. They most probably expect 0 as 'no-value', and suddenly it's -1.

I see three options:

1.Bump cxx11_test to require C++17, and use std::optional everywhere, including the public headers. 2.Keep the public headers C++11-compatible: use std::optional in the internal implementation, and in the public structs use a plain value plus a bool flag. 3.Add a benchmark::Optional that is the same for all standards (no #if) and use it for every complexity_n.

Since this is more of a policy decision, I’d rather not pick one myself. Which would you prefer?

I think it shouldn't be 1.
I guess 3., but just make a strict ComplexityN type that has a method to check if it has a value (internally an <0 check), and a getter that panics if the value isn't there.

[amoralpha]

Please update all complexity_n/complexity_n_ fields to all be Optional<>. There should not be any == -1/<0/>=0 checks.

Thanks for the review. I ran into an issue making everything std::optional, so I’d like to ask for your input before going further.
ThreadManager::Result is internal, so std::optional works fine there. The problem is State::complexity_n_ and BenchmarkReporter::Run::complexity_n, which are in public headers. cxx11_test builds those headers in C++11/14 mode, where doesn’t exist, so it breaks (MSVC: 'optional': is not a member of 'std'). Everything else builds fine.

Oh well, i forgot about that.

Also, changing the type of the public Run::complexity_n would break any custom reporter that reads it as an integer.

So here's the thing, this diff already breaks that. They most probably expect 0 as 'no-value', and suddenly it's -1.

I see three options:
1.Bump cxx11_test to require C++17, and use std::optional everywhere, including the public headers. 2.Keep the public headers C++11-compatible: use std::optional in the internal implementation, and in the public structs use a plain value plus a bool flag. 3.Add a benchmark::Optional that is the same for all standards (no #if) and use it for every complexity_n.
Since this is more of a policy decision, I’d rather not pick one myself. Which would you prefer?

I think it shouldn't be 1. I guess 3., but just make a strict ComplexityN type that has a method to check if it has a value (internally an <0 check), and a getter that panics if the value isn't there.

Got it, I've implemented ComplexityN locally as a strict class in include/benchmark/statistics.h (default-constructed as unset with n_ = -1, has_value() doing the >= 0 check, and value() aborting if unset, plus aborting on negative construction), and updated State::complexity_n_, ThreadManager::Result::complexity_n, and BenchmarkReporter::Run::complexity_n to use it with no external -1 / < 0 / >= 0 checks.

One quick question before I push: since ComplexityN is now a class rather than an integer typedef, what would you prefer for BigOFunc (typedef double(BigOFunc)(...)) and MinimalLeastSq?

1. Keep BigOFunc taking int64_t so existing user lambdas (e.g. [](int64_t n) -> double) keep compiling without changes.
2. Have BigOFunc take ComplexityN (callers/lambdas access n.value()).

[LebedevRI]

Code around

    BM_CHECK_GE(run.complexity_n, 0)
        << "Did you forget to call SetComplexityN?";

would desugar the new type into a valid int64_t.
I think the existing ComplexityN type alias should stay the way it is with the existing name.

[amoralpha]

Code around

    BM_CHECK_GE(run.complexity_n, 0)
        << "Did you forget to call SetComplexityN?";

would desugar the new type into a valid int64_t. I think the existing ComplexityN type alias should stay the way it is with the existing name.

Thanks! I've pushed this. ComplexityN stays the int64_t alias, and BigOFunc, MinimalLeastSq, the fitting curves, the tests and the user guide are unchanged.

The strict type is a separate OptionalComplexityN (default-constructed means "no value", has_value(), and value() aborts if unset; constructing it from a negative number also aborts). It is used only for State::complexity_n_, ThreadManager::Result::complexity_n and BenchmarkReporter::Run::complexity_n. ComputeBigO checks has_value() and unwraps it into a plain ComplexityN, as you described.

Please let me know if you'd like a different name. The Python bindings (State.complexity_n) are updated too, but they aren't compiled in my local CMake setup, so CI will be the first real build of that part.

[LebedevRI]

Nice, this looks somewhat reasonable.
I'll need to sleep on this, but i do believe this will be fine overall.
Thank you.

[LebedevRI]

Can these be in the header?

[amoralpha]

include/benchmark/statistics.h is a public installed header (and compiled under C++11 in cxx11_test), whereas BM_CHECK lives in internal src/check.h (which isn't installed and uses C++17 std::string_view), so BM_CHECK can only be used from .cc files under src/.

[LebedevRI]

Right. No idea why i said BM_CHECK. These should just be assert(), like in #2274.

[amoralpha]

I've updated OptionalComplexityN to be header-only using assert() just like PR #2274, removed the definitions from src/statistics.cc, and switched to ASSERT_DEBUG_DEATH for the tests. Tested it locally and all 87 tests pass smoothly in both debug and release.
Thanks for pointing that out and bearing with me! Let me know if it looks good now.

[LebedevRI]

Never seen that bazel failure before:

ERROR: /home/runner/work/benchmark/benchmark/BUILD.bazel:41:11: Compiling src/reporter.cc failed: absolute path inclusion(s) found in rule '//:benchmark':
the source file 'src/reporter.cc' includes the following non-builtin files with absolute paths (if these are builtin files, make sure these paths are in your toolchain):
  '/usr/lib/gcc/x86_64-linux-gnu/15/include/stddef.h'
  '/usr/lib/gcc/x86_64-linux-gnu/15/include/stdarg.h'
  '/usr/lib/gcc/x86_64-linux-gnu/15/include/stdint.h'

The actual source file seems to vary between runs.
@dmah42 Does bzl version needs to be bumped, because underlying compiler version is too new for it to be aware of it?

[amoralpha]

Hi @LebedevRI just a follow up - any other change you want me to do on my side? thx!

[LebedevRI]

I do not understand why the CI check is failing here, but not in other PR's/branches.

[amoralpha]

Could you re-run the earlier green bazel.ubuntu-latest job from this PR (commit f079722)? If it fails now, it’s the runner i think?

[LebedevRI]

@dmah42 halp. That workflow is marked as required and github does not allow me to merge this without it passing.

[dmah42]

i've been working on this, sorry. it's because there's a rollout changing "-latest" between two versions and our bazel cache is version-agnostic.

[amoralpha]

Thank you @LebedevRI and @dmah42, I really appreciate the detailed reviews and your patience with the CI issues. I learned a lot from this one.
If there are other open issues you think would suit a newer contributor, I’d be glad to take a look. Is there a preferred place for contributor questions (issues, Discussions, or somewhere else)?

[LebedevRI]

@amoralpha thank you!

@dmah42 thanks!