Skip to content
This repository has been archived by the owner on Jun 17, 2022. It is now read-only.

Remove deprecated std pair wrappers #11

Merged

Conversation

jnewbery
Copy link

@jnewbery jnewbery commented Jan 8, 2018

These are deprecated and no longer used in Bitcoin Core.

@jnewbery jnewbery changed the title Remote deprecated std pair wrappers Remove deprecated std pair wrappers Jan 8, 2018
Copy link

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

utACK 4f3d7f4

@jnewbery jnewbery force-pushed the remote_deprecated_std_pair_wrappers branch from f6e2da0 to 50cd05a Compare January 15, 2018 14:18
@jnewbery
Copy link
Author

jnewbery commented Jan 15, 2018

Rebased now that #11 has been merged. @MarcoFalke - do you mind reviewing? Should be trivial.

@jnewbery jnewbery force-pushed the remote_deprecated_std_pair_wrappers branch from 50cd05a to 85052a4 Compare February 13, 2018 20:38
@jnewbery
Copy link
Author

jnewbery commented May 3, 2018

Should be trivially reviewable. Just apply this patch to src/univalue in the bitcoin/bitcoin repo and verify that it still builds.

@maflcko
Copy link

maflcko commented May 3, 2018

utACK 85052a4

@maflcko
Copy link

maflcko commented May 3, 2018

Just noting for completeness, that this would break other downstream projects (if there are any). Effectively we'd have to tag a release before this breaking change and then mark this as a breaking change when tagged a version that includes this change.

Though, it doesn't seem we do proper releases (or tags) at all, so I think this is nothing to worry about.

@Empact
Copy link

Empact commented Aug 23, 2018

utACK 85052a4

@jnewbery
Copy link
Author

@achow101, @fanquake, @instagibbs, @sipa - this would have prevented the push_back(Pair()) call being introduced that was removed in bitcoin/bitcoin#13927. Since you all reviewed that PR, would you mind taking a look at this? It should be a straightforward review - you can test by applying the change to the univalue subtree in bitcoin and then verifying that the build completes. Thanks!

@maflcko
Copy link

maflcko commented Aug 31, 2018

Imo this is ready for merge

@achow101
Copy link
Member

ACK 85052a4

@fanquake
Copy link
Member

utACK 85052a4

@jnewbery
Copy link
Author

jnewbery commented Sep 4, 2018

@laanwj - I think this is ready for merge

@laanwj laanwj merged commit 85052a4 into bitcoin-core:master Sep 7, 2018
laanwj added a commit that referenced this pull request Sep 7, 2018
85052a4 Remove deprecated std::pair wrappers (Karel Bilek)

Pull request description:

  These are deprecated and no longer used in Bitcoin Core.

Tree-SHA512: 774c9b84f88e0c5e2062b3d7f03399d9909830d044bc3cf8dfcd45e58f48072ce527f471360cb36594e898e625d965b71436559bff2bb4b368255b9f1e26f722
@luke-jr
Copy link
Member

luke-jr commented Sep 7, 2018

Uh, NACK?

As @MarcoFalke noted, this breaks the API...

This isn't a Bitcoin Core project. Releases are upstream.

@maflcko
Copy link

maflcko commented Sep 7, 2018

There is no univalue release with these changes included, so there is nothing broken. We'd only have to "[...] mark this as a breaking change when tagged a version that includes this change."

Note that the subtree bump in the bitcoin core repo is only a "stylistic" change to prevent devs from using the deprecated wrappers. (Sort of a linter)

@jnewbery jnewbery deleted the remote_deprecated_std_pair_wrappers branch December 20, 2018 15:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
9 participants