Skip to content

refactor: Remove WithParams serialization helper, use SER_PARAMS_OPFUNC#28503

Merged
fanquake merged 3 commits intobitcoin:masterfrom
maflcko:2309-nuke-with-params-force-opfunc-
Oct 31, 2023
Merged

refactor: Remove WithParams serialization helper, use SER_PARAMS_OPFUNC#28503
fanquake merged 3 commits intobitcoin:masterfrom
maflcko:2309-nuke-with-params-force-opfunc-

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Sep 19, 2023

Every serialization parameter struct already has the SER_PARAMS_OPFUNC, except for one in the tests.

For consistency, and to remove verbose code, convert the test to SER_PARAMS_OPFUNC, and use it everywhere, then remove the WithParams helper.

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 19, 2023 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK ajtowns, TheCharlatan
Concept ACK theuni

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #28451 (Remove unused SER_DISK, SER_NETWORK, SER_GETHASH by maflcko)

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.

@ajtowns ajtowns mentioned this pull request Sep 21, 2023
Merged
@fanquake
Copy link
Member

fanquake commented Oct 2, 2023

cc @theuni

@maflcko maflcko force-pushed the 2309-nuke-with-params-force-opfunc- branch from fac9afe to fa544f8 Compare October 3, 2023 09:56
@theuni
Copy link
Member

theuni commented Oct 4, 2023

Concept ACK.

MarcoFalke added 2 commits October 13, 2023 10:30
test: Use SER_PARAMS_OPFUNC in serialize_tests.cpp
fae9054
scripted-diff: Use ser params operator
ffffb4a 
-BEGIN VERIFY SCRIPT-
 sed -i 's|WithParams(\([a-zA-Z:._]\+\), |\1(|g' $( git grep -l WithParams )
-END VERIFY SCRIPT-
@maflcko maflcko force-pushed the 2309-nuke-with-params-force-opfunc- branch from fa544f8 to fa664dc Compare October 13, 2023 08:32
@ajtowns ajtowns mentioned this pull request Oct 27, 2023
Merged
@maflcko maflcko requested a review from sedited October 27, 2023 07:57
sedited
sedited approved these changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@sedited sedited left a comment

Choose a reason for hiding this comment

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

ACK fa664dc335f02c392e3268f403f32bcf5bd9eeee

@DrahtBot DrahtBot requested a review from theuni October 30, 2023 10:38
@fanquake fanquake requested a review from ajtowns October 30, 2023 11:23
ajtowns
ajtowns reviewed Oct 30, 2023
View reviewed changes
Copy link
Contributor

@ajtowns ajtowns left a comment

Choose a reason for hiding this comment

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

ACK fa664dc335f02c392e3268f403f32bcf5bd9eeee

@maflcko maflcko force-pushed the 2309-nuke-with-params-force-opfunc- branch from fa664dc to 9999019 Compare October 30, 2023 12:55
@maflcko maflcko requested a review from ajtowns October 30, 2023 14:22
@DrahtBot DrahtBot mentioned this pull request Oct 30, 2023
Merged
@ajtowns
Copy link
Contributor

ajtowns commented Oct 31, 2023

reACK 9999019

@DrahtBot DrahtBot requested review from sedited and removed request for ajtowns October 31, 2023 04:25
sedited
sedited approved these changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@sedited sedited left a comment

Choose a reason for hiding this comment

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

Re-ACK 9999019

@fanquake fanquake merged commit d51fb9c into bitcoin:master Oct 31, 2023
@maflcko maflcko deleted the 2309-nuke-with-params-force-opfunc- branch November 1, 2023 07:13
@ryanofsky ryanofsky mentioned this pull request Nov 21, 2023
Draft
@bitcoin bitcoin locked and limited conversation to collaborators Dec 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sedited sedited sedited approved these changes

@theuni theuni Awaiting requested review from theuni

+1 more reviewer

@ajtowns ajtowns ajtowns left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@maflcko @DrahtBot @fanquake @theuni @ajtowns @sedited