Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Feb 3, 2022

The insert and erase methods have many issues:

Fix all issues by removing them

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 4, 2022

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #24231 (streams: Fix read-past-the-end and integer overflows by MarcoFalke)

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.

Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

ACK, though I'd put the first commit last so the other two can be backported* cleaner.

Also confirmed 21.x doesn't use these functions either, so a real fix isn't needed either.

* I don't see a need to backport them, though.

@maflcko
Copy link
Member Author

maflcko commented Feb 8, 2022

I don't see a need to backport them, though.

Indeed, there is no need to backport, so I am going to leave as is for now.

@laanwj
Copy link
Member

laanwj commented Feb 9, 2022

Code review ACK fa1b227
Nice cleanup!

@laanwj laanwj merged commit 5e8e0b3 into bitcoin:master Feb 9, 2022
@maflcko maflcko deleted the 2202-s branch February 9, 2022 16:15
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 12, 2022
fa1b227 Remove broken and unused CDataStream methods (MarcoFalke)
faee5f8 test: Create fresh CDataStream each time (MarcoFalke)
fa71114 test: Inline expected_xor (MarcoFalke)

Pull request description:

  The `insert` and `erase` methods have many issues:

  * They are unused
  * They are confusing and hard to read, as they implement "special cases" for optimization, that isn't needed
  * They are broken (See bitcoin#24231)
  * Fixing them leads to mingw compile errors (See bitcoin#24231 (comment))

  Fix all issues by removing them

ACKs for top commit:
  laanwj:
    Code review ACK fa1b227

Tree-SHA512: 9d9e5d42e6ffc5ae82bdb67cfb5b50b45977ae674acee6ff99092560aebf2fc7e4584ded614e190db0663226fa198e34350517cd7ee57d518de22e9568bc349a
@bitcoin bitcoin locked and limited conversation to collaborators Feb 9, 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.

4 participants