From e51e2996070f27c86203b99f19847e5a25f2a73c Mon Sep 17 00:00:00 2001 From: Lin Huang Date: Mon, 9 Sep 2024 10:07:52 -0400 Subject: [PATCH 1/7] Verify index >= 0 in apply_diff and fix warnings --- libraries/libfc/include/fc/container/ordered_diff.hpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/libraries/libfc/include/fc/container/ordered_diff.hpp b/libraries/libfc/include/fc/container/ordered_diff.hpp index d028ad7a97..4ae554b458 100644 --- a/libraries/libfc/include/fc/container/ordered_diff.hpp +++ b/libraries/libfc/include/fc/container/ordered_diff.hpp @@ -108,15 +108,17 @@ class ordered_diff { // Remove from the source based on diff.remove_indexes std::ptrdiff_t offset = 0; for (SizeType index : diff.remove_indexes) { - FC_ASSERT(index + offset < container.size(), "diff.remove_indexes index ${idx} + offset ${o} not in range ${s}", + SizeType updated_index = index + offset; + // Safe to do static_cast as `updated_index >= 0` is verified first + FC_ASSERT(updated_index >= 0 && (static_cast::size_type>(updated_index) < container.size()), "diff.remove_indexes index ${idx} + offset ${o} not in range ${s}", ("idx", index)("o", offset)("s", container.size())); - container.erase(container.begin() + index + offset); + container.erase(container.begin() + updated_index); --offset; } // Insert into the source based on diff.insert_indexes for (auto& [index, value] : diff.insert_indexes) { - FC_ASSERT(index <= container.size(), "diff.insert_indexes index ${idx} not in range ${s}", + FC_ASSERT(index >= 0 && static_cast::size_type>(index) <= container.size(), "diff.insert_indexes index ${idx} not in range ${s}", ("idx", index)("s", container.size())); container.insert(container.begin() + index, std::move(value)); } From d47bf544db227ccbaa5df194671ca58107809a55 Mon Sep 17 00:00:00 2001 From: Lin Huang Date: Mon, 9 Sep 2024 13:17:40 -0400 Subject: [PATCH 2/7] Use auto instead of SizeType for updated_index --- libraries/libfc/include/fc/container/ordered_diff.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/libfc/include/fc/container/ordered_diff.hpp b/libraries/libfc/include/fc/container/ordered_diff.hpp index 4ae554b458..38c5ebd247 100644 --- a/libraries/libfc/include/fc/container/ordered_diff.hpp +++ b/libraries/libfc/include/fc/container/ordered_diff.hpp @@ -108,7 +108,7 @@ class ordered_diff { // Remove from the source based on diff.remove_indexes std::ptrdiff_t offset = 0; for (SizeType index : diff.remove_indexes) { - SizeType updated_index = index + offset; + auto updated_index = index + offset; // Safe to do static_cast as `updated_index >= 0` is verified first FC_ASSERT(updated_index >= 0 && (static_cast::size_type>(updated_index) < container.size()), "diff.remove_indexes index ${idx} + offset ${o} not in range ${s}", ("idx", index)("o", offset)("s", container.size())); From 52f470a58f754b9dec65d998862dc21f7a129842 Mon Sep 17 00:00:00 2001 From: Lin Huang Date: Mon, 9 Sep 2024 21:54:37 -0400 Subject: [PATCH 3/7] Enfore indices are strictly monotonically increasing and clean up FC_ASSERTS to get rid of compiler warnings --- .../include/fc/container/ordered_diff.hpp | 30 ++++++++++++------- libraries/libfc/test/test_ordered_diff.cpp | 8 ++--- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/libraries/libfc/include/fc/container/ordered_diff.hpp b/libraries/libfc/include/fc/container/ordered_diff.hpp index 38c5ebd247..df1cd2d9c9 100644 --- a/libraries/libfc/include/fc/container/ordered_diff.hpp +++ b/libraries/libfc/include/fc/container/ordered_diff.hpp @@ -26,7 +26,10 @@ namespace fc { * @param Container container type for ordered diff and for diff_result */ template typename Container = std::vector> -requires std::equality_comparable && std::random_access_iterator::iterator> +requires std::equality_comparable + && std::random_access_iterator::iterator> + && std::is_unsigned::value + && (std::numeric_limits::size_type>::max() >= std::numeric_limits::max()) class ordered_diff { public: using size_type = SizeType; @@ -106,19 +109,26 @@ class ordered_diff { requires std::same_as, diff_result> static Container apply_diff(Container&& container, X&& diff) { // Remove from the source based on diff.remove_indexes - std::ptrdiff_t offset = 0; - for (SizeType index : diff.remove_indexes) { - auto updated_index = index + offset; - // Safe to do static_cast as `updated_index >= 0` is verified first - FC_ASSERT(updated_index >= 0 && (static_cast::size_type>(updated_index) < container.size()), "diff.remove_indexes index ${idx} + offset ${o} not in range ${s}", - ("idx", index)("o", offset)("s", container.size())); + for (typename Container::size_type i = 0; i < diff.remove_indexes.size(); ++i) { + FC_ASSERT(i == 0 || diff.remove_indexes[i] > diff.remove_indexes[i-1], + "diff.remove_indexes not strictly monotonically increasing: current index ${c}, previous index ${p}", + ("c", diff.remove_indexes[i])("p", diff.remove_indexes[i-1])); + + assert(diff.remove_indexes[i] >= i); + assert(std::numeric_limits::size_type>::min() + i <= diff.remove_indexes[i]); // check for underflow of diff.remove_indexes[i] - i + auto updated_index = diff.remove_indexes[i] - i; + FC_ASSERT(updated_index < container.size(), "diff.remove_indexes index ${idx} - i ${i} not in range ${s}", + ("idx", diff.remove_indexes[i])("i", i)("s", container.size())); container.erase(container.begin() + updated_index); - --offset; } // Insert into the source based on diff.insert_indexes - for (auto& [index, value] : diff.insert_indexes) { - FC_ASSERT(index >= 0 && static_cast::size_type>(index) <= container.size(), "diff.insert_indexes index ${idx} not in range ${s}", + for (typename Container::size_type i = 0; i < diff.insert_indexes.size(); ++i) { + FC_ASSERT(i == 0 || diff.insert_indexes[i].first > diff.insert_indexes[i-1].first, + "diff.insert_indexes not strictly monotonically increasing: current index ${c}, previous index ${p}", + ("c", diff.insert_indexes[i].first)("p", diff.insert_indexes[i-1].first)); + auto& [index, value] = diff.insert_indexes[i]; + FC_ASSERT(index <= container.size(), "diff.insert_indexes index ${idx} not in range ${s}", ("idx", index)("s", container.size())); container.insert(container.begin() + index, std::move(value)); } diff --git a/libraries/libfc/test/test_ordered_diff.cpp b/libraries/libfc/test/test_ordered_diff.cpp index eeb9687dd3..745eb6b7af 100644 --- a/libraries/libfc/test/test_ordered_diff.cpp +++ b/libraries/libfc/test/test_ordered_diff.cpp @@ -38,15 +38,15 @@ BOOST_AUTO_TEST_CASE(ordered_diff_test) try { { // All elements removed vector source = {'a', 'b', 'c', 'd', 'e'}; vector target; - auto result = ordered_diff::diff(source, target); - source = ordered_diff::apply_diff(std::move(source), result); + auto result = ordered_diff::diff(source, target); + source = ordered_diff::apply_diff(std::move(source), result); BOOST_TEST(source == target); } { // All elements removed, size 1 vector source = {'a'}; vector target; - auto result = ordered_diff::diff(source, target); - source = ordered_diff::apply_diff(std::move(source), result); + auto result = ordered_diff::diff(source, target); + source = ordered_diff::apply_diff(std::move(source), result); BOOST_TEST(source == target); } { // All elements inserted From 417b8af7367d489fb638269bbc9872876abcec61 Mon Sep 17 00:00:00 2001 From: Lin Huang Date: Tue, 10 Sep 2024 08:30:36 -0400 Subject: [PATCH 4/7] define container_size_type --- libraries/libfc/include/fc/container/ordered_diff.hpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/libraries/libfc/include/fc/container/ordered_diff.hpp b/libraries/libfc/include/fc/container/ordered_diff.hpp index df1cd2d9c9..e6f6bc3cbc 100644 --- a/libraries/libfc/include/fc/container/ordered_diff.hpp +++ b/libraries/libfc/include/fc/container/ordered_diff.hpp @@ -28,11 +28,12 @@ namespace fc { template typename Container = std::vector> requires std::equality_comparable && std::random_access_iterator::iterator> - && std::is_unsigned::value + && std::is_unsigned_v && (std::numeric_limits::size_type>::max() >= std::numeric_limits::max()) class ordered_diff { public: using size_type = SizeType; + using container_size_type = typename Container::size_type; struct diff_result { Container remove_indexes; @@ -109,13 +110,13 @@ class ordered_diff { requires std::same_as, diff_result> static Container apply_diff(Container&& container, X&& diff) { // Remove from the source based on diff.remove_indexes - for (typename Container::size_type i = 0; i < diff.remove_indexes.size(); ++i) { + for (container_size_type i = 0; i < diff.remove_indexes.size(); ++i) { FC_ASSERT(i == 0 || diff.remove_indexes[i] > diff.remove_indexes[i-1], "diff.remove_indexes not strictly monotonically increasing: current index ${c}, previous index ${p}", ("c", diff.remove_indexes[i])("p", diff.remove_indexes[i-1])); assert(diff.remove_indexes[i] >= i); - assert(std::numeric_limits::size_type>::min() + i <= diff.remove_indexes[i]); // check for underflow of diff.remove_indexes[i] - i + assert(std::numeric_limits::min() + i <= diff.remove_indexes[i]); // check for underflow of diff.remove_indexes[i] - i auto updated_index = diff.remove_indexes[i] - i; FC_ASSERT(updated_index < container.size(), "diff.remove_indexes index ${idx} - i ${i} not in range ${s}", ("idx", diff.remove_indexes[i])("i", i)("s", container.size())); @@ -123,7 +124,7 @@ class ordered_diff { } // Insert into the source based on diff.insert_indexes - for (typename Container::size_type i = 0; i < diff.insert_indexes.size(); ++i) { + for (container_size_type i = 0; i < diff.insert_indexes.size(); ++i) { FC_ASSERT(i == 0 || diff.insert_indexes[i].first > diff.insert_indexes[i-1].first, "diff.insert_indexes not strictly monotonically increasing: current index ${c}, previous index ${p}", ("c", diff.insert_indexes[i].first)("p", diff.insert_indexes[i-1].first)); From 8d1f2fe05eb509e64f5f02503b508137c05b5d1d Mon Sep 17 00:00:00 2001 From: Lin Huang Date: Tue, 10 Sep 2024 08:31:43 -0400 Subject: [PATCH 5/7] Remove a duplicate assert --- libraries/libfc/include/fc/container/ordered_diff.hpp | 1 - 1 file changed, 1 deletion(-) diff --git a/libraries/libfc/include/fc/container/ordered_diff.hpp b/libraries/libfc/include/fc/container/ordered_diff.hpp index e6f6bc3cbc..35ef9c9984 100644 --- a/libraries/libfc/include/fc/container/ordered_diff.hpp +++ b/libraries/libfc/include/fc/container/ordered_diff.hpp @@ -116,7 +116,6 @@ class ordered_diff { ("c", diff.remove_indexes[i])("p", diff.remove_indexes[i-1])); assert(diff.remove_indexes[i] >= i); - assert(std::numeric_limits::min() + i <= diff.remove_indexes[i]); // check for underflow of diff.remove_indexes[i] - i auto updated_index = diff.remove_indexes[i] - i; FC_ASSERT(updated_index < container.size(), "diff.remove_indexes index ${idx} - i ${i} not in range ${s}", ("idx", diff.remove_indexes[i])("i", i)("s", container.size())); From 1f6be3e002f0356c722fb15fee03f310abfad613 Mon Sep 17 00:00:00 2001 From: Lin Huang Date: Tue, 10 Sep 2024 14:25:37 -0400 Subject: [PATCH 6/7] Ensure Container::size_type is unsigned --- libraries/libfc/include/fc/container/ordered_diff.hpp | 1 + 1 file changed, 1 insertion(+) diff --git a/libraries/libfc/include/fc/container/ordered_diff.hpp b/libraries/libfc/include/fc/container/ordered_diff.hpp index 35ef9c9984..46c8ec147d 100644 --- a/libraries/libfc/include/fc/container/ordered_diff.hpp +++ b/libraries/libfc/include/fc/container/ordered_diff.hpp @@ -29,6 +29,7 @@ template && std::random_access_iterator::iterator> && std::is_unsigned_v + && std::is_unsigned_v::size_type> && (std::numeric_limits::size_type>::max() >= std::numeric_limits::max()) class ordered_diff { public: From 2f18e3c8ef457c444e9aeb4c7d9b8312a42484f1 Mon Sep 17 00:00:00 2001 From: Lin Huang Date: Tue, 10 Sep 2024 14:26:42 -0400 Subject: [PATCH 7/7] std::forward diff argument in apply_diff() --- libraries/libfc/include/fc/container/ordered_diff.hpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/libraries/libfc/include/fc/container/ordered_diff.hpp b/libraries/libfc/include/fc/container/ordered_diff.hpp index 46c8ec147d..b89371bb9a 100644 --- a/libraries/libfc/include/fc/container/ordered_diff.hpp +++ b/libraries/libfc/include/fc/container/ordered_diff.hpp @@ -104,12 +104,14 @@ class ordered_diff { return result; } - /// @param diff the diff_result created from diff(source, target), apply_diff(std::move(source), diff_result) => target + /// @param diff_in (input) the diff_result created from diff(source, target), apply_diff(std::move(source), diff_result) => target /// @param container the source of diff(source, target) to modify using the diff_result to produce original target /// @return the modified container now equal to original target template requires std::same_as, diff_result> - static Container apply_diff(Container&& container, X&& diff) { + static Container apply_diff(Container&& container, X&& diff_in) { + X diff = std::forward(diff_in); + // Remove from the source based on diff.remove_indexes for (container_size_type i = 0; i < diff.remove_indexes.size(); ++i) { FC_ASSERT(i == 0 || diff.remove_indexes[i] > diff.remove_indexes[i-1],