Page MenuHomePhabricator

Fix UniValue .write() changes for C++98
ClosedPublic

Authored by jasonbcox on Jun 3 2020, 23:31.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Commits
rABCda311f5ed8fe: Fix UniValue .write() changes for C++98
Summary

While C++14 is enforced throughout our CMake build, it is not enforced
for every subproject in the Autotools build. As a result, D5982 breaks the build
when UniValue is built with C++98 (thereby affecting Autotools builds on Xenial).
Normally, we wouldn't care because the Autotools build is no longer supported and
we enforce C++14. However, the UniValue library does not yet officially support
C++11 or later, so keeping our changes consistent with our upstream PRs will help
reduce maintenance costs in the long run.

This patch is the same I applied to my current upstream PR for the .write() perf improvements:
https://github.com/jgarzik/univalue/compare/d3a09e5e4a48417d5478a66dc343d7bf00146d5a..bb1c002bf540851551b33e76ad208baaf7ea5b11

While removing the std::move() call is not ideal, it does not appear to impact performance
in an easily measurable way.

Test Plan

Make sure there are no errors or warnings in the CMake build:

mkdir build && cd build
cmake -GNinja ..
ninja check check-univalue

Make sure the Autotools build works as expected (I ran this on both Debian 10 and Ubuntu 16.04):

./autogen.sh
mkdir build && cd build
../configure
make check

Make sure UniValue builds with C++98:

cd src/univalue
./autogen.sh
mkdir build && cd build
CXXFLAGS="-std=c++98" ../configure
make check

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

deadalnix requested changes to this revision.Jun 4 2020, 00:35
deadalnix added a subscriber: deadalnix.

Removing the move makes no sense It's going to generate a copy of the whole serialized thing.

This revision now requires changes to proceed.Jun 4 2020, 00:35

Introduce preprocessor directive to allow compilation with std::move. This change differs from
the upstream PR for now, as a strategy for C++11 in UniValue will take more time.

deadalnix added inline comments.
src/univalue/lib/univalue_write.cpp
25 ↗(On Diff #20906)

This is valid C++98:

std::string ret;
std::swap(ret, str);
return ret;

You might want to considered upstreaming this.

This revision is now accepted and ready to land.Jun 4 2020, 17:53

Use swap to emulate std::move for C++98

Fabien added inline comments.
src/univalue/lib/univalue_write.cpp
55 ↗(On Diff #20911)

Should be size_t

This revision was automatically updated to reflect the committed changes.