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

refactor: Remove unused CDataStream::rdbuf method #26258

Merged
merged 1 commit into from
Oct 10, 2022

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Oct 5, 2022

It is unused and seems unlikely to be ever used.

It is unused and seems unlikely to be ever used.
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

Code-review ACK fabbbe3

Seems like this method was introduced by Satoshi and hasn't been used for >10 years (last use was removed with commit 5ce4c2a) has never been used 👀

Copy link
Contributor

@aureleoules aureleoules left a comment

Choose a reason for hiding this comment

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

ACK fabbbe3

Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

Compact() also seems unused, can remove that too?

@maflcko
Copy link
Member Author

maflcko commented Oct 5, 2022

Seems like this method was introduced by Satoshi and hasn't been used for >10 years (last use was removed with commit 5ce4c2a) eyes

Was this ever used? Looks like the commit you link to was a call to ip::tcp::iostream::rdbuf(), which may not be CDataStream, but rather https://www.boost.org/doc/libs/1_80_0/doc/html/boost_asio/reference/basic_socket_iostream/rdbuf.html

@maflcko
Copy link
Member Author

maflcko commented Oct 5, 2022

Compact() also seems unused, can remove that too?

As opposed to rdbuf one could imagine a use case for this, so I kept it for now. But happy to drop as well if reviewers want that.

@theStack
Copy link
Contributor

theStack commented Oct 5, 2022

Seems like this method was introduced by Satoshi and hasn't been used for >10 years (last use was removed with commit 5ce4c2a) eyes

Was this ever used? Looks like the commit you link to was a call to ip::tcp::iostream::rdbuf(), which may not be CDataStream, but rather https://www.boost.org/doc/libs/1_80_0/doc/html/boost_asio/reference/basic_socket_iostream/rdbuf.html

Oh you're right, it seems the method you removed was indeed never used then 🤔

@fanquake fanquake merged commit cf3db7c into bitcoin:master Oct 10, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 10, 2022
fabbbe3 Remove unused CDataStream::rdbuf method (MacroFake)

Pull request description:

  It is unused and seems unlikely to be ever used.

ACKs for top commit:
  theStack:
    Code-review ACK fabbbe3
  aureleoules:
    ACK fabbbe3

Tree-SHA512: 5804642658f96a0fb51482ebf3a062bb0f997c1e0527455afa4aceeeb6c1ad139a98b14a7c8a0909daba733a83bdc24fcadad45060ead4be6eb3dc3e66c129e2
@maflcko maflcko deleted the 2210-no-rdbuf-🍥 branch October 11, 2022 15:05
@bitcoin bitcoin locked and limited conversation to collaborators Oct 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants