search

jgarzik / univalue

High performance RAII C++ JSON library and universal value object class
MIT License
56 stars 77 forks source link

Support move-semantics for UniValue #52

Open domob1812 opened 6 years ago

domob1812 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

domob1812 commented 6 years ago

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 commented 6 years ago

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

martinus commented 3 years ago

Any updates on that issue? Is c++98 still a requirement? My PR https://github.com/jgarzik/univalue/pull/79 introduces move semantics which can lead to a very significant speedup of about a factor of 2.

jasonbcox commented 3 years ago

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.