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

Removing deprecated std::pair wrappers #5

Closed
karelbilek wants to merge 2 commits into from
Closed

Removing deprecated std::pair wrappers #5

karelbilek wants to merge 2 commits into from

Conversation

@karelbilek
Copy link

@karelbilek karelbilek commented Sep 24, 2017

This removes std::pair wrappers to simplify API

Note: this requires changes in bitcoin core, as in bitcoin/bitcoin#11386

Not sure where to backport this test change, or if it's even possible - jgarzik@6e71669

@karelbilek karelbilek mentioned this pull request Sep 24, 2017
Closed
sipa
sipa reviewed Sep 24, 2017
View reviewed changes
include/univalue.h Outdated
return pushKV(key, tmpVal);
}
bool pushKV(const std::string& key, bool val_) {
UniValue tmpVal((bool)val_);
Copy link

@sipa sipa Sep 24, 2017

Choose a reason for hiding this comment

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

cast is unnecessary

Copy link
Author

@karelbilek karelbilek Sep 24, 2017

Choose a reason for hiding this comment

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

Oh yes, result of copy-paste.

@jnewbery
Copy link

jnewbery commented Dec 12, 2017

It'd be nice if this could add a test case for the new code. The master branch now has an object.cpp test, where you could add a new test case. Take a look at https://github.com/jnewbery/univalue/tree/pr5.1 . It adds a test case for this change, along with a bunch of clean ups to the test file and the internal representation of bools (I think it's worth taking that change to the internal representation so we don't add a test that tests the current internal representation).

If you think those changes are good, feel free to reset this branch to jnewbery@91cbfeb

@jnewbery jnewbery mentioned this pull request Dec 19, 2017
Merged
@jnewbery
Copy link

jnewbery commented Dec 19, 2017

@karel-3d - I didn't see an update to this PR for a week so I've taken the liberty of opening #10, which includes your commits plus a test. I'm happy to maintain #10 going forward, but also happy to close if you want to pick this up again.

@karelbilek
Copy link
Author

karelbilek commented Dec 19, 2017

OK, I am closing this one, since I don't have the time to maintain it.

@karelbilek karelbilek closed this Dec 19, 2017
@jnewbery
Copy link

jnewbery commented Dec 19, 2017

Thanks for your commits @karel-3d ! I'll maintain this in #10.

@jnewbery jnewbery mentioned this pull request Jan 8, 2018
Merged
laanwj added a commit that referenced this pull request Jan 13, 2018
@laanwj
Merge #10: Add pushKV(key, boolean) function (replaces #5)
51d3ab3 
129bad9 [tests] test pushKV for boolean values (John Newbery)
b3c44c9 Pushing boolean value to univalue correctly (Karel Bilek)

Pull request description:

  Adds the new pushKV(key, boolean) function from #5, with test cases.

  Removal of srd::pair wrappers is in a future PR.

Tree-SHA512: d13b377365181723c06d4e4d7efa88f3cdbc6ca06ec2effe2ddf834bdfc7e602072c957296370323cb4e921a6934d55b040ebbdacca1dbff66e75d32073e6ac8
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@sipa sipa sipa left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@karelbilek @jnewbery @sipa