-
Notifications
You must be signed in to change notification settings - Fork 37.7k
fuzz: Flatten all FUZZ_TARGET macros into one #28065
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
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
fad67d8
to
fae353d
Compare
Concept ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
Adding a new option requires doubling the number of existing macros in the worst case.
I've run into this before and it is indeed pretty annoying.
Thanks for the comments. Fixed everything. |
fa35b0c
to
fa6fa1f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK fa6fa1f
fa64a9f
to
fa34d1f
Compare
* This allows to reduce the number of total macros. * Also, adding a new option no longer requires doubling the number of macros in the worst case.
-BEGIN VERIFY SCRIPT- ren() { sed --regexp-extended -i "s|$1|$2|g" $(git grep -l --extended-regexp "$1"); } # Replace FUZZ_TARGET_INIT ren 'FUZZ_TARGET_INIT\((.+), (.+)\)' 'FUZZ_TARGET(\1, .init = \2)' # Delete unused FUZZ_TARGET_INIT sed -i -e '37,39d' src/test/fuzz/fuzz.h -END VERIFY SCRIPT-
fa34d1f
to
fa6dfaa
Compare
{ | ||
const auto it_ins = FuzzTargets().try_emplace(name, std::move(target), std::move(init), hidden); | ||
const auto it_ins{FuzzTargets().try_emplace(name, FuzzTarget /* temporary can be dropped in C++20 */ {std::move(target), std::move(opts)})}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const auto it_ins{FuzzTargets().try_emplace(name, FuzzTarget /* temporary can be dropped in C++20 */ {std::move(target), std::move(opts)})}; | |
const auto it_ins{FuzzTargets().try_emplace(name, std::move(target), std::move(opts))}; |
Haven't tracked down which C++20 LWG paper this was. Let me know if someone finds something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK fa6dfaa
fa6dfaa scripted-diff: Use new FUZZ_TARGET macro everywhere (MarcoFalke) fa36ad8 fuzz: Accept options in FUZZ_TARGET macro (MarcoFalke) Pull request description: The `FUZZ_TARGET` macros have many issues: * The developer will have to pick the right macro to pass the wanted option. * Adding a new option requires doubling the number of existing macros in the worst case. Fix all issues by using only a single macro. This refactor does not change behavior. ACKs for top commit: dergoegge: ACK fa6dfaa Tree-SHA512: 49a34553867a1734ce89e616b2d7c29b784a67cd8990db6573f0c7b18957636ef0c81d3d0d444a04c12cdc98bc4c4aa7a2ec94e6232dc363620a746e28416444
The
FUZZ_TARGET
macros have many issues:Fix all issues by using only a single macro.
This refactor does not change behavior.