From c8c16de16793a8dc037965d2177c776212b11844 Mon Sep 17 00:00:00 2001 From: Vladimir Glavnyy <31897320+vglavnyy@users.noreply.github.com> Date: Mon, 10 May 2021 23:07:56 +0700 Subject: [PATCH] Fix reverse iterators for Vector and Array (#6626) This commit fixes two serious issues connected with reverse iterators: 1. Vector's rbegin/rend was incompatible with std::reverse_iterator::base(); 2. Array's rend() was incorrect, it based on end() instead of begin(). --- include/flatbuffers/flatbuffers.h | 26 ++++++++---- tests/test.cpp | 66 ++++++++++++++++++++++++++++++- 2 files changed, 84 insertions(+), 8 deletions(-) diff --git a/include/flatbuffers/flatbuffers.h b/include/flatbuffers/flatbuffers.h index bc4255c2e..ee34d54ed 100644 --- a/include/flatbuffers/flatbuffers.h +++ b/include/flatbuffers/flatbuffers.h @@ -172,8 +172,12 @@ template struct VectorIterator { return (data_ - other.data_) / IndirectHelper::element_stride; } + // Note: return type is incompatible with the standard + // `reference operator*()`. IT operator*() const { return IndirectHelper::Read(data_, 0); } + // Note: return type is incompatible with the standard + // `pointer operator->()`. IT operator->() const { return IndirectHelper::Read(data_, 0); } VectorIterator &operator++() { @@ -227,12 +231,18 @@ struct VectorReverseIterator : public std::reverse_iterator { explicit VectorReverseIterator(Iterator iter) : std::reverse_iterator(iter) {} + // Note: return type is incompatible with the standard + // `reference operator*()`. typename Iterator::value_type operator*() const { - return *(std::reverse_iterator::current); + auto tmp = std::reverse_iterator::current; + return *--tmp; } + // Note: return type is incompatible with the standard + // `pointer operator->()`. typename Iterator::value_type operator->() const { - return *(std::reverse_iterator::current); + auto tmp = std::reverse_iterator::current; + return *--tmp; } }; @@ -295,14 +305,14 @@ template class Vector { iterator end() { return iterator(Data(), size()); } const_iterator end() const { return const_iterator(Data(), size()); } - reverse_iterator rbegin() { return reverse_iterator(end() - 1); } + reverse_iterator rbegin() { return reverse_iterator(end()); } const_reverse_iterator rbegin() const { - return const_reverse_iterator(end() - 1); + return const_reverse_iterator(end()); } - reverse_iterator rend() { return reverse_iterator(begin() - 1); } + reverse_iterator rend() { return reverse_iterator(begin()); } const_reverse_iterator rend() const { - return const_reverse_iterator(begin() - 1); + return const_reverse_iterator(begin()); } const_iterator cbegin() const { return begin(); } @@ -463,7 +473,9 @@ template class Array { const_reverse_iterator rbegin() const { return const_reverse_iterator(end()); } - const_reverse_iterator rend() const { return const_reverse_iterator(end()); } + const_reverse_iterator rend() const { + return const_reverse_iterator(begin()); + } const_iterator cbegin() const { return begin(); } const_iterator cend() const { return end(); } diff --git a/tests/test.cpp b/tests/test.cpp index c649faf59..2c0033979 100644 --- a/tests/test.cpp +++ b/tests/test.cpp @@ -290,7 +290,7 @@ void AccessFlatBufferTest(const uint8_t *flatbuf, size_t length, unsigned char inv_data[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 }; // Check compatibilty of iterators with STL. std::vector inv_vec(inventory->begin(), inventory->end()); - int n = 0; + size_t n = 0; for (auto it = inventory->begin(); it != inventory->end(); ++it, ++n) { auto indx = it - inventory->begin(); TEST_EQ(*it, inv_vec.at(indx)); // Use bounds-check. @@ -3848,6 +3848,69 @@ void ParseIncorrectMonsterJsonTest() { TEST_EQ(parser.ParseJson("{name:+f}"), false); } +#if !defined(_MSC_VER) || _MSC_VER >= 1700 +template +void TestIterators(const std::vector &expected, const Container &tested) { + TEST_ASSERT(tested.rbegin().base() == tested.end()); + TEST_ASSERT(tested.crbegin().base() == tested.cend()); + TEST_ASSERT(tested.rend().base() == tested.begin()); + TEST_ASSERT(tested.crend().base() == tested.cbegin()); + + size_t k = 0; + for (auto it = tested.begin(); it != tested.end(); ++it, ++k) { + const auto &e = expected.at(k); + TEST_EQ(*it, e); + } + TEST_EQ(k, expected.size()); + + k = expected.size(); + for (auto it = tested.rbegin(); it != tested.rend(); ++it, --k) { + const auto &e = expected.at(k - 1); + TEST_EQ(*it, e); + } + TEST_EQ(k, 0); +} + +void FlatbuffersIteratorsTest() { + { + flatbuffers::FlatBufferBuilder fbb; + const std::vector inv_data = { 1, 2, 3 }; + { + auto mon_name = fbb.CreateString("MyMonster"); // key, mandatory + auto inv_vec = fbb.CreateVector(inv_data); + auto empty_i64_vec = + fbb.CreateVector(static_cast(nullptr), 0); + MonsterBuilder mb(fbb); + mb.add_name(mon_name); + mb.add_inventory(inv_vec); + mb.add_vector_of_longs(empty_i64_vec); + FinishMonsterBuffer(fbb, mb.Finish()); + } + const auto &mon = *flatbuffers::GetRoot(fbb.GetBufferPointer()); + + TEST_EQ_STR("MyMonster", mon.name()->c_str()); + TEST_ASSERT(mon.inventory()); + TEST_ASSERT(mon.vector_of_longs()); + TestIterators(inv_data, *mon.inventory()); + TestIterators(std::vector(), *mon.vector_of_longs()); + } + + { + flatbuffers::FlatBufferBuilder fbb; + MyGame::Example::ArrayStruct aStruct; + MyGame::Example::FinishArrayTableBuffer( + fbb, MyGame::Example::CreateArrayTable(fbb, &aStruct)); + const auto &array_table = + *flatbuffers::GetRoot(fbb.GetBufferPointer()); + TEST_ASSERT(array_table.a()); + auto &int_15 = *array_table.a()->b(); + TestIterators(std::vector(15, 0), int_15); + } +} +#else +void FlatbuffersIteratorsTest() {} +#endif + int FlatBufferTests() { // clang-format off @@ -3944,6 +4007,7 @@ int FlatBufferTests() { StringVectorDefaultsTest(); ParseIncorrectMonsterJsonTest(); FlexBuffersFloatingPointTest(); + FlatbuffersIteratorsTest(); return 0; }