From bdf0db974acccf6b52491bbadd859c9c38e15c70 Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Tue, 28 Apr 2015 09:59:36 +0200 Subject: [PATCH 1/4] compound: Swap order of enum elements The order doesn't matter for correctness, but now "yes" will show as "1" and "no" as "0" in GDB (it doesn't seem to resolve enum values to names). --- compound.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compound.hh b/compound.hh index bb7f02a9b0..ab333748bb 100644 --- a/compound.hh +++ b/compound.hh @@ -32,7 +32,7 @@ struct value_traits { } }; -enum class allow_prefixes { yes, no }; +enum class allow_prefixes { no, yes }; template class compound_type final { From 923aca98f4caba5a5bb9607a6e9d42b010318719 Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Tue, 28 Apr 2015 10:53:26 +0200 Subject: [PATCH 2/4] compound: Fix handling of empty components in prefixable compounds We didn't handle properly the case when the last component of a prefixable compound was empty. Because we do not encode component's length, we did not distinguish a compound with last element empty from a compound without the last element. The fix is to always encode lengths in prefixable tuples. --- compound.hh | 45 ++++++++++++++++++++++----------------------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/compound.hh b/compound.hh index ab333748bb..4d25778d4b 100644 --- a/compound.hh +++ b/compound.hh @@ -66,8 +66,8 @@ public: * Format: * ...(len(value_n))? * - * The value corresponding to the last component of types doesn't have its length encoded, - * its length is deduced from the input range. + * For non-prefixable compounds, the value corresponding to the last component of types doesn't + * have its length encoded, its length is deduced from the input range. * * serialize_value() and serialize_optionals() for single element rely on the fact that for a single-element * compounds their serialized form is equal to the serialized form of the component. @@ -84,7 +84,7 @@ public: for (auto&& wrapped : values) { auto&& val = value_traits::unwrap(wrapped); assert(val.size() <= std::numeric_limits::max()); - if (--n_left) { + if (--n_left || AllowPrefixes == allow_prefixes::yes) { write(out, uint16_t(val.size())); } out = std::copy(val.begin(), val.end(), out); @@ -97,7 +97,7 @@ public: for (auto&& wrapped : values) { auto&& val = value_traits::unwrap(wrapped); assert(val.size() <= std::numeric_limits::max()); - if (--n_left) { + if (--n_left || AllowPrefixes == allow_prefixes::yes) { len += sizeof(uint16_t); } len += val.size(); @@ -107,21 +107,20 @@ public: bytes serialize_single(bytes&& v) { if (AllowPrefixes == allow_prefixes::no) { assert(_types.size() == 1); + return std::move(v); } else { - if (_types.size() > 1) { - std::vector vec; - vec.reserve(1); - vec.emplace_back(std::move(v)); - return ::serialize_value(*this, vec); - } + // FIXME: Optimize + std::vector vec; + vec.reserve(1); + vec.emplace_back(std::move(v)); + return ::serialize_value(*this, vec); } - return std::move(v); } bytes serialize_value(const std::vector& values) { return ::serialize_value(*this, values); } bytes serialize_value(std::vector&& values) { - if (_types.size() == 1 && values.size() == 1) { + if (AllowPrefixes == allow_prefixes::no && _types.size() == 1 && values.size() == 1) { return std::move(values[0]); } return ::serialize_value(*this, values); @@ -130,7 +129,7 @@ public: return ::serialize_value(*this, values); } bytes serialize_optionals(std::vector&& values) { - if (_types.size() == 1 && values.size() == 1) { + if (AllowPrefixes == allow_prefixes::no && _types.size() == 1 && values.size() == 1) { assert(values[0]); return std::move(*values[0]); } @@ -164,18 +163,19 @@ public: _v = bytes_view(nullptr, 0); return; } - if (_v.empty()) { - if (AllowPrefixes == allow_prefixes::yes) { - _v = bytes_view(nullptr, 0); - return; - } else { - throw marshal_exception(); - } - } + --_types_left; uint16_t len; - if (_types_left == 1) { + if (_types_left == 0 && AllowPrefixes == allow_prefixes::no) { len = _v.size(); } else { + if (_v.empty()) { + if (AllowPrefixes == allow_prefixes::yes) { + _v = bytes_view(nullptr, 0); + return; + } else { + throw marshal_exception(); + } + } len = read_simple(_v); if (_v.size() < len) { throw marshal_exception(); @@ -191,7 +191,6 @@ public: } iterator(end_iterator_tag, const bytes_view& v) : _v(nullptr, 0) {} iterator& operator++() { - --_types_left; read_current(); return *this; } From 4c2390348e3e832f4ca866585b285b494ba10abd Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Tue, 28 Apr 2015 10:56:53 +0200 Subject: [PATCH 3/4] tests: Introduce range_assert Makes it wasier to express requirements about ranges. --- tests/urchin/range_assert.hh | 43 ++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 tests/urchin/range_assert.hh diff --git a/tests/urchin/range_assert.hh b/tests/urchin/range_assert.hh new file mode 100644 index 0000000000..a6dd29c53e --- /dev/null +++ b/tests/urchin/range_assert.hh @@ -0,0 +1,43 @@ +/* + * Copyright 2015 Cloudius Systems + */ + +#pragma once + +#include +#include + +template +class range_assert { + using value_type = typename Iterator::value_type; + Iterator _begin; + Iterator _end; +public: + range_assert(Iterator begin, Iterator end) + : _begin(begin) + , _end(end) + { } + + template + range_assert equals(std::vector expected) { + auto i = _begin; + auto expected_i = expected.begin(); + while (i != _end && expected_i != expected.end()) { + BOOST_REQUIRE(*i == *expected_i); + ++i; + ++expected_i; + } + if (i != _end) { + BOOST_FAIL("Expected fewer elements"); + } + if (expected_i != expected.end()) { + BOOST_FAIL("Expected more elements"); + } + return *this; + } +}; + +template +static range_assert assert_that_range(Iterator begin, Iterator end) { + return { begin, end }; +} From b4c400612a6d2dc219fe3f0e67d3ac9af630c9d1 Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Tue, 28 Apr 2015 10:57:40 +0200 Subject: [PATCH 4/4] tests: Introduce compound_test --- configure.py | 1 + test.py | 1 + tests/urchin/compound_test.cc | 148 ++++++++++++++++++++++++++++++++++ 3 files changed, 150 insertions(+) create mode 100644 tests/urchin/compound_test.cc diff --git a/configure.py b/configure.py index be6a4b619e..1a7089f0c4 100755 --- a/configure.py +++ b/configure.py @@ -169,6 +169,7 @@ urchin_tests = [ 'tests/urchin/serializer_test', 'tests/urchin/message', 'tests/urchin/gossip', + 'tests/urchin/compound_test', ] tests = [ diff --git a/test.py b/test.py index 72fc9821c6..2498ae9725 100755 --- a/test.py +++ b/test.py @@ -41,6 +41,7 @@ boost_tests = [ 'test-serialization', 'cartesian_product_test', 'urchin/UUID_test', + 'urchin/compound_test', ] other_tests = [ diff --git a/tests/urchin/compound_test.cc b/tests/urchin/compound_test.cc new file mode 100644 index 0000000000..1f06cdea89 --- /dev/null +++ b/tests/urchin/compound_test.cc @@ -0,0 +1,148 @@ +/* + * Copyright 2015 Cloudius Systems + */ + +#define BOOST_TEST_DYN_LINK +#define BOOST_TEST_MODULE core + +#include +#include "compound.hh" +#include "tests/urchin/range_assert.hh" + +static std::vector to_bytes_vec(std::vector values) { + std::vector result; + for (auto&& v : values) { + result.emplace_back(to_bytes(v)); + } + return result; +} + +template +static +range_assert +assert_that_components(Compound& t, bytes packed) { + return assert_that_range(t.begin(packed), t.end(packed)); +} + +template +static void test_sequence(Compound& t, std::vector strings) { + auto packed = t.serialize_value(to_bytes_vec(strings)); + assert_that_components(t, packed).equals(to_bytes_vec(strings)); +}; + +BOOST_AUTO_TEST_CASE(test_iteration_over_non_prefixable_tuple) { + compound_type t({bytes_type, bytes_type, bytes_type}); + + test_sequence(t, {"el1", "el2", "el3"}); + test_sequence(t, {"el1", "el2", ""}); + test_sequence(t, {"", "el2", "el3"}); + test_sequence(t, {"el1", "", ""}); + test_sequence(t, {"", "", "el3"}); + test_sequence(t, {"el1", "", "el3"}); + test_sequence(t, {"", "", ""}); +} + +BOOST_AUTO_TEST_CASE(test_iteration_over_prefixable_tuple) { + compound_type t({bytes_type, bytes_type, bytes_type}); + + test_sequence(t, {"el1", "el2", "el3"}); + test_sequence(t, {"el1", "el2", ""}); + test_sequence(t, {"", "el2", "el3"}); + test_sequence(t, {"el1", "", ""}); + test_sequence(t, {"", "", "el3"}); + test_sequence(t, {"el1", "", "el3"}); + test_sequence(t, {"", "", ""}); + + test_sequence(t, {"el1", "el2", ""}); + test_sequence(t, {"el1", "el2"}); + test_sequence(t, {"el1", ""}); + test_sequence(t, {"el1"}); + test_sequence(t, {""}); + test_sequence(t, {}); +} + +BOOST_AUTO_TEST_CASE(test_iteration_over_non_prefixable_singular_tuple) { + compound_type t({bytes_type}); + + test_sequence(t, {"el1"}); + test_sequence(t, {""}); +} + +BOOST_AUTO_TEST_CASE(test_iteration_over_prefixable_singular_tuple) { + compound_type t({bytes_type}); + + test_sequence(t, {"elem1"}); + test_sequence(t, {""}); + test_sequence(t, {}); +} + +template +void do_test_conversion_methods_for_singular_compound() { + compound_type t({bytes_type}); + + { + assert_that_components(t, t.serialize_value(to_bytes_vec({"asd"}))) // r-value version + .equals(to_bytes_vec({"asd"})); + } + + { + auto vec = to_bytes_vec({"asd"}); + assert_that_components(t, t.serialize_value(vec)) // l-value version + .equals(to_bytes_vec({"asd"})); + } + + { + std::vector vec = { bytes_opt("asd") }; + assert_that_components(t, t.serialize_optionals(vec)) + .equals(to_bytes_vec({"asd"})); + } + + { + std::vector vec = { bytes_opt("asd") }; + assert_that_components(t, t.serialize_optionals(std::move(vec))) // r-value + .equals(to_bytes_vec({"asd"})); + } + + { + assert_that_components(t, t.serialize_single(bytes("asd"))) + .equals(to_bytes_vec({"asd"})); + } +} + +BOOST_AUTO_TEST_CASE(test_conversion_methods_for_singular_compound) { + do_test_conversion_methods_for_singular_compound(); + do_test_conversion_methods_for_singular_compound(); +} + +template +void do_test_conversion_methods_for_non_singular_compound() { + compound_type t({bytes_type, bytes_type, bytes_type}); + + { + assert_that_components(t, t.serialize_value(to_bytes_vec({"el1", "el2", "el2"}))) // r-value version + .equals(to_bytes_vec({"el1", "el2", "el2"})); + } + + { + auto vec = to_bytes_vec({"el1", "el2", "el3"}); + assert_that_components(t, t.serialize_value(vec)) // l-value version + .equals(to_bytes_vec({"el1", "el2", "el3"})); + } + + { + std::vector vec = { bytes_opt("el1"), bytes_opt("el2"), bytes_opt("el3") }; + assert_that_components(t, t.serialize_optionals(vec)) + .equals(to_bytes_vec({"el1", "el2", "el3"})); + } + + { + std::vector vec = { bytes_opt("el1"), bytes_opt("el2"), bytes_opt("el3") }; + assert_that_components(t, t.serialize_optionals(std::move(vec))) // r-value + .equals(to_bytes_vec({"el1", "el2", "el3"})); + } +} + +BOOST_AUTO_TEST_CASE(test_conversion_methods_for_non_singular_compound) { + do_test_conversion_methods_for_non_singular_compound(); + do_test_conversion_methods_for_non_singular_compound(); +}