Skip to content

Conversation

@promag
Copy link
Contributor

@promag promag commented May 7, 2019

Since UniValue constructors aren't marked explicit then these overloaded auxiliary members are unnecessary. This change also avoids some unnecessary UniValue copies.

Related to #15974.

@luke-jr
Copy link
Member

luke-jr commented May 7, 2019

NACK modifying UniValue here. Change it upstream or not at all, please.

@promag
Copy link
Contributor Author

promag commented May 7, 2019

@luke-jr I know and will do (I've prefixed with univalue: because of that). I'm just submitting this because of the required changes here.

@luke-jr
Copy link
Member

luke-jr commented May 7, 2019

The fact that it requires changes to our code would imply the overloaded members aren't actually unnecessary...?

@laanwj
Copy link
Member

laanwj commented Jun 6, 2019

I don't think this makes sense, sorry. If we're not using the overloaded methods doesn't mean other clients of univalue don't, so that's not enough reason to remove them.

@laanwj laanwj closed this Jun 6, 2019
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
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