Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for custom information per benchmark run #77

Merged
merged 4 commits into from
Nov 9, 2022

Conversation

jonas-schulze
Copy link
Contributor

@jonas-schulze jonas-schulze commented Nov 7, 2022

  • Set via bench.context("name", "value")
  • Render/access via {{context(name)}}
  • Rendering variables without a value produce empty output (for that variable) throws an exception
  • Values persist to subsequent calls to Bench::run; my use case assumes that all variables are needed for all runs
  • Reset via bench.clearContext()

Unfortunately, I haven't been able to build and therefore check the docs I wrote.

Also, I saw that some methods are overloaded for char const* and std::string const&. Should Bench::context do the same? What about Result::context?

Do you think context is a feasible name? If not, how should these methods be called?

@jonas-schulze
Copy link
Contributor Author

Ubuntu hirsute has reached EOL, so I guess the CI failure is ok?

Regardning AppVeyor, I have no experience with Windoes, so don't know how to address the error message.
I added a member to Config, which may be the reason, but I wonder why it built just fine on the (recent) Ubuntus.

'ankerl::nanobench::Config::Config(ankerl::nanobench::Config &&) noexcept': is not a special member function which can be defaulted

Copy link
Owner

@martinus martinus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, that's a nice feature, I like it! Thanks for the contribution. I have some minor comments.

I think context name is ok, at least I don't have one that I think is better.

about const char* overloads, please add one, and the implementation can just call the std::string version then.

src/include/nanobench.h Outdated Show resolved Hide resolved
src/include/nanobench.h Outdated Show resolved Hide resolved
@jonas-schulze
Copy link
Contributor Author

about const char* overloads, please add one, and the implementation can just call the std::string version then.

Given that there is a constructor std::string(char const*), this conversion should happen implicitly when calling context(std::string const&,...), shouldn't it? Passing a char const* to it works just fine. I am not opposed to add the overloads, I just want to understand their benefits.

Some more observations:

  • Some overloads are not DRY, e.g. Bench::name
  • Some methods calls are the other way around, i.e. std::string const& to char const*, e.g. Bench::unit and Bench::render (search for c_str)

Is that something that should be addressed?

src/include/nanobench.h Outdated Show resolved Hide resolved
@jonas-schulze
Copy link
Contributor Author

I addressed all open issues. This is ready for review again. 🙂

@martinus martinus merged commit 7845eaa into martinus:master Nov 9, 2022
@martinus
Copy link
Owner

martinus commented Nov 9, 2022

Thanks a lot!

jonas-schulze added a commit to jonas-schulze/nanobench that referenced this pull request Nov 18, 2022
@jonas-schulze jonas-schulze mentioned this pull request Nov 18, 2022
martinus pushed a commit that referenced this pull request Dec 3, 2022
* Fix some typos

* Fix some more typos (#77 follow-up)

* Add/Fix link to Bench::context
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants