Skip to content
This repository has been archived by the owner on Feb 15, 2024. It is now read-only.

Support move-semantics for UniValue #52

Open
domob1812 opened this issue Jul 31, 2018 · 8 comments
Open

Support move-semantics for UniValue #52

domob1812 opened this issue Jul 31, 2018 · 8 comments

Comments

@domob1812
Copy link

UniValue as a class could take advantage of C++11 move semantics to improve efficiency in some cases. (It is probably not very relevant in practice and for Bitcoin Core, but would still be nice to have.)

What do you think, would that make sense to add? (If yes, then I'm happy to provide a PR for it.)

@domob1812
Copy link
Author

Note also that univalue seems to be not yet built with C++11 (at least it looked like that when I last played around). Is that by design (in which case this issue would be invalid) or just because it has not been changed yet?

@jgarzik
Copy link
Owner

jgarzik commented Aug 10, 2018

Note also that univalue seems to be not yet built with C++11 (at least it looked like that when I last played around). Is that by design (in which case this issue would be invalid) or just because it has not been changed yet?

  1. Building with C++11 is ok and just has not yet been done yet.

  2. Would like to continue support for pre-C++11

@domob1812
Copy link
Author

That makes sense. For move constructors and assignment, it would be easy to make them conditional on C++11 with some preprocessor macro.

What do you think about that? If you think it makes sense, I can give it a try.

@jgarzik
Copy link
Owner

jgarzik commented Aug 10, 2018

@domob1812 Sounds good to me. The main difficulty I imagine will be crafting a test that proves the code compiles absent C++11 :)

@domob1812
Copy link
Author

Ok, let me try. :) Yes, having an automated test for that is probably hard or impossible - but if all the new code is protected by preprocessor conditionals and we run a manual test or so, I hope it is still fine. (But let's just see what I can come up with.)

@jgarzik
Copy link
Owner

jgarzik commented Aug 10, 2018

My guess would be forcing the dialect via -std=c++98 or -std=c++0x in a special Travis compile matrix

@martinus
Copy link

Any updates on that issue? Is c++98 still a requirement? My PR #79 introduces move semantics which can lead to a very significant speedup of about a factor of 2.

@jasonbcox
Copy link
Contributor

I had a discussion with Jeff on this issue about 6 months ago. While some projects like Bitcoin Core and Bitcoin ABC support C++11 or later, there was a concern that Ubuntu 16 LTS and other distros have already shipped older versions of univalue and older compilers, making it difficult to assess impact on "in the field" users.

However, Ubuntu 16 LTS is reaching EOL in a few months. This may be worth revisiting.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants