Skip to content
New issue

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

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

Already on GitHub? # to your account

Clarify the difference between BENCHMARK_TEMPLATE_F and BENCHMARK_TEMPLATE_DEFINE_F + BENCHMARK_REGISTER_F #1815

Merged
merged 3 commits into from
Jul 16, 2024

Conversation

jiawen
Copy link
Contributor

@jiawen jiawen commented Jul 15, 2024

Add comments highlighting the difference between BENCHMARK_TEMPLATE_F and BENCHMARK_TEMPLATE_DEFINE_F, mirroring those of BENCHMARK_F and BENCHMARK_DEFINE_F.

Add comments highlighting the difference between `BENCHMARK_TEMPLATE_F` and `BENCHMARK_TEMPLATE_DEFINE_F`, mirroring those of `BENCHMARK_F ` and `BENCHMARK_DEFINE_F`.
@@ -665,7 +665,9 @@ BENCHMARK_TEMPLATE_DEFINE_F(MyFixture, DoubleTest, double)(benchmark::State& st)
}
}

/* DoubleTest is NOT registered */
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think //-style comments are used elsewhere, so let's be consistent.
I think it would be good to also mention IntTest,
is BENCHMARK_TEMPLATE_F enough, and only the BENCHMARK_TEMPLATE_DEFINE_F
needs to be followed up with BENCHMARK_REGISTER_F or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍, I'm happy to do that (and will write some tests to confirm!).

In the user guide, Fixtures is similar and uses /* ... */ comments - I think it's within scope to update that one.

Shall I update the one other instance in Preventing Optimization as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍, I'm happy to do that (and will write some tests to confirm!).

In the user guide, Fixtures is similar and uses /* ... */ comments - I think it's within scope to update that one.

Shall I update the one other instance in Preventing Optimization as well?

Eh, i guess why not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. PTAL.

@jiawen jiawen changed the title Clarify BENCHMARK_REGISTER_F Clarify the difference between BENCHMARK_TEMPLATE_F and BENCHMARK_TEMPLATE_DEFINE_F + BENCHMARK_REGISTER_F Jul 15, 2024
@jiawen jiawen requested a review from LebedevRI July 15, 2024 23:55
@LebedevRI
Copy link
Collaborator

Seems nice to me. I did not verify that the behavior of BENCHMARK_F/BENCHMARK_TEMPLATE_F
is as stated, but it does seem reasonable.

@dmah42 dmah42 merged commit d2cd246 into google:main Jul 16, 2024
80 checks passed
@dmah42
Copy link
Member

dmah42 commented Jul 16, 2024

great. thank you!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants