Skip to content

Add AutoFile without ser-type and ser-version and use it where possible #25285

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 20, 2022
Merged

Add AutoFile without ser-type and ser-version and use it where possible #25285

merged 2 commits into from
Jul 20, 2022

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jun 6, 2022

This was done in the context of #25284 , but I think it also makes sense standalone.

The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.

So do this here for AutoFile. CAutoFile remains in places where it is not yet possible.

@laanwj
Copy link
Member

laanwj commented Jun 6, 2022

Concept ACK

@DrahtBot DrahtBot mentioned this pull request Jun 7, 2022
Merged
2 tasks
@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 7, 2022 •
edited
Loading

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

Conflicts

No conflicts as of last run.

This was referenced Jun 7, 2022
Merged
Merged
@DrahtBot DrahtBot mentioned this pull request Jun 7, 2022
Merged
@DrahtBot DrahtBot mentioned this pull request Jun 7, 2022
Closed
This was referenced Jun 21, 2022
Merged
Closed
MacroFake added 2 commits June 29, 2022 10:31
streams: Add AutoFile without ser-type and ser-version
6666803 
The moved parts can be reviewed with "--color-moved=dimmed-zebra".
The one-char changes can be reviewed with "--word-diff-regex=.".
Use AutoFile where possible
facc2fa
@laanwj
Copy link
Member

laanwj commented Jun 29, 2022

Code review ACK facc2fa
The code change is straightforward and easy to review.

However, I am not entirely happy about the naming of AutoFile versus CAutoFile I would personally prefer a more distinctive name.

@maflcko
Copy link
Member Author

maflcko commented Jun 29, 2022

However, I am not entirely happy about the naming of AutoFile versus CAutoFile I would personally prefer a more distinctive name.

CAutoFile is deprecated, only used in two remaining places: addrdb and tx/block serialization, and will be removed as a follow-up to #25284. So I left the name as is, but I am happy to append a scripted-diff here to rename CAutoFile to something else. Maybe LegacyAutoFile?

@laanwj
Copy link
Member

laanwj commented Jun 29, 2022

OK, if it's going away soon™ i have no problem with this. There's something to be said for an intermediate rename, but also there's drawbacks in generating a bigger diff.

I just imagined it's pretty frustrating for developers to have similarly named classes with slightly different behavior, it's not something to keep on the long run (it reminds me of the situation in net classes naming CAddress / CService / CNetAddr a bit, where, even though i've worked on this code for years keep forgetting what is what).

@maflcko maflcko requested a review from ryanofsky July 19, 2022 10:37
fanquake
fanquake approved these changes Jul 20, 2022
View reviewed changes
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK facc2fa

@fanquake fanquake merged commit 895937e into bitcoin:master Jul 20, 2022
@maflcko maflcko deleted the 2206-autofile-🌟 branch July 20, 2022 13:32
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 20, 2022
@fanquake
Merge bitcoin#25285: Add AutoFile without ser-type and ser-version an…
9dfa0a3 
…d use i…
@bitcoin bitcoin locked and limited conversation to collaborators Jul 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fanquake fanquake fanquake approved these changes

@ryanofsky ryanofsky Awaiting requested review from ryanofsky

Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maflcko @laanwj @DrahtBot @fanquake