From e1e7791ff8a34d1dc09f758d48d8b29efb54f64d Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Thu, 8 Sep 2022 16:43:57 -0700 Subject: [PATCH] [FOLD] Review feedback from @scottschurr: * Use an explicit function to combine arrays instead of an operator+ * Rename a few variables for consistency --- src/test/app/AmendmentTable_test.cpp | 106 ++++++++++++++++++--------- 1 file changed, 71 insertions(+), 35 deletions(-) diff --git a/src/test/app/AmendmentTable_test.cpp b/src/test/app/AmendmentTable_test.cpp index c7f9932133d..4284190a18a 100644 --- a/src/test/app/AmendmentTable_test.cpp +++ b/src/test/app/AmendmentTable_test.cpp @@ -35,21 +35,6 @@ namespace ripple { -// Even though this is written as a template, and could potentially be a -// global helper, it's only intended to be used for -// std::vector in this class. -template -std::vector -operator+(std::vector left, std::vector right) -{ - left.reserve(left.size() + right.size()); - for (auto& item : right) - { - left.emplace_back(std::move(item)); - } - return left; -} - class AmendmentTable_test final : public beast::unit_test::suite { private: @@ -141,6 +126,44 @@ class AmendmentTable_test final : public beast::unit_test::suite return makeFeatureInfo(amendments, VoteBehavior::Obsolete); } + template + static size_t + totalsize(std::vector const& src, Args const&... args) + { + if constexpr (sizeof...(args) > 0) + return src.size() + totalsize(args...); + return src.size(); + } + + template + static void + combine_arg( + std::vector& dest, + std::vector const& src, + Args const&... args) + { + assert(dest.capacity() >= dest.size() + src.size()); + std::copy(src.begin(), src.end(), std::back_inserter(dest)); + if constexpr (sizeof...(args) > 0) + combine_arg(dest, args...); + } + + template + static std::vector + combine( + // Pass "left" by value. The values will need to be copied one way or + // another, so just reuse it. + std::vector left, + std::vector const& right, + Args const&... args) + { + left.reserve(totalsize(left, right, args...)); + + combine_arg(left, right, args...); + + return left; + } + // All useful amendments are supported amendments. // Enabled amendments are typically a subset of supported amendments. // Vetoed amendments should be supported but not enabled. @@ -152,17 +175,17 @@ class AmendmentTable_test final : public beast::unit_test::suite std::vector const vetoed_{"a", "c", "e"}; std::vector const obsolete_{"0", "1", "2"}; std::vector const allSupported_{ - yes_ + enabled_ + vetoed_ + obsolete_}; + combine(yes_, enabled_, vetoed_, obsolete_)}; std::vector const unsupported_{"v", "w", "x"}; std::vector const unsupportedMajority_{"y", "z"}; - Section const emptySection; - std::vector const emptyYes; + Section const emptySection_; + std::vector const emptyYes_; - test::SuiteJournal journal; + test::SuiteJournal journal_; public: - AmendmentTable_test() : journal("AmendmentTable_test", *this) + AmendmentTable_test() : journal_("AmendmentTable_test", *this) { } @@ -175,7 +198,7 @@ class AmendmentTable_test final : public beast::unit_test::suite Section const& vetoed) { return make_AmendmentTable( - app, majorityTime, supported, enabled, vetoed, journal); + app, majorityTime, supported, enabled, vetoed, journal_); } std::unique_ptr @@ -192,11 +215,20 @@ class AmendmentTable_test final : public beast::unit_test::suite std::unique_ptr makeTable(test::jtx::Env& env, std::chrono::seconds majorityTime) { + static std::vector const supported = + combine( + makeDefaultYes(yes_), + // Use non-intuitive default votes for "enabled_" and "vetoed_" + // so that when the tests later explicitly enable or veto them, + // we can be certain that they are not simply going by their + // default vote setting. + makeDefaultNo(enabled_), + makeDefaultYes(vetoed_), + makeObsolete(obsolete_)); return makeTable( env.app(), majorityTime, - makeDefaultYes(yes_) + makeDefaultNo(enabled_) + - makeDefaultYes(vetoed_) + makeObsolete(obsolete_), + supported, makeSection(enabled_), makeSection(vetoed_)); } @@ -285,7 +317,7 @@ class AmendmentTable_test final : public beast::unit_test::suite try { test::jtx::Env env{*this, makeConfig()}; - if (makeTable(env, weeks(2), yesVotes, test, emptySection)) + if (makeTable(env, weeks(2), yesVotes, test, emptySection_)) fail("Accepted only amendment ID"); } catch (std::exception const& e) @@ -302,7 +334,7 @@ class AmendmentTable_test final : public beast::unit_test::suite try { test::jtx::Env env{*this, makeConfig()}; - if (makeTable(env, weeks(2), yesVotes, test, emptySection)) + if (makeTable(env, weeks(2), yesVotes, test, emptySection_)) fail("Accepted extra arguments"); } catch (std::exception const& e) @@ -323,7 +355,7 @@ class AmendmentTable_test final : public beast::unit_test::suite try { test::jtx::Env env{*this, makeConfig()}; - if (makeTable(env, weeks(2), yesVotes, test, emptySection)) + if (makeTable(env, weeks(2), yesVotes, test, emptySection_)) fail("Accepted short amendment ID"); } catch (std::exception const& e) @@ -343,7 +375,7 @@ class AmendmentTable_test final : public beast::unit_test::suite try { test::jtx::Env env{*this, makeConfig()}; - if (makeTable(env, weeks(2), yesVotes, test, emptySection)) + if (makeTable(env, weeks(2), yesVotes, test, emptySection_)) fail("Accepted long amendment ID"); } catch (std::exception const& e) @@ -364,7 +396,7 @@ class AmendmentTable_test final : public beast::unit_test::suite try { test::jtx::Env env{*this, makeConfig()}; - if (makeTable(env, weeks(2), yesVotes, test, emptySection)) + if (makeTable(env, weeks(2), yesVotes, test, emptySection_)) fail("Accepted non-hex amendment ID"); } catch (std::exception const& e) @@ -577,7 +609,7 @@ class AmendmentTable_test final : public beast::unit_test::suite test::jtx::Env env{*this}; auto table = - makeTable(env, weeks(2), emptyYes, emptySection, emptySection); + makeTable(env, weeks(2), emptyYes_, emptySection_, emptySection_); std::vector> votes; std::vector ourVotes; @@ -638,7 +670,11 @@ class AmendmentTable_test final : public beast::unit_test::suite test::jtx::Env env{*this}; auto table = makeTable( - env, weeks(2), emptyYes, emptySection, makeSection(testAmendment)); + env, + weeks(2), + emptyYes_, + emptySection_, + makeSection(testAmendment)); auto const validators = makeValidators(10); @@ -697,7 +733,7 @@ class AmendmentTable_test final : public beast::unit_test::suite test::jtx::Env env{*this}; auto table = makeTable( - env, weeks(2), makeDefaultYes(yes_), emptySection, emptySection); + env, weeks(2), makeDefaultYes(yes_), emptySection_, emptySection_); auto const validators = makeValidators(10); std::vector> votes; @@ -780,8 +816,8 @@ class AmendmentTable_test final : public beast::unit_test::suite env, weeks(2), makeDefaultYes(testAmendment), - emptySection, - emptySection); + emptySection_, + emptySection_); auto const validators = makeValidators(16); @@ -851,8 +887,8 @@ class AmendmentTable_test final : public beast::unit_test::suite env, weeks(8), makeDefaultYes(testAmendment), - emptySection, - emptySection); + emptySection_, + emptySection_); std::set enabled; majorityAmendments_t majority;