From 28f22828d2bd196da0ca3ca9982b9cca1b8a6c1c Mon Sep 17 00:00:00 2001 From: Charles Lechasseur Date: Tue, 21 Nov 2017 11:44:36 -0500 Subject: [PATCH] Fix dangling reference issues Some operators created iterators, used them to return a reference then destroyed the iterator. This does not mesh well with sequences whose iterators return references to data in the iterator, like those returned by select(). Fixed by returning value types instead. --- lib/coveo/linq/detail/linq_detail.h | 30 +++--- lib/coveo/linq/linq.h | 4 +- tests/coveo/linq/all_tests.cpp | 1 + tests/coveo/linq/linq_tests.cpp | 143 +++++++++++++++++++--------- tests/coveo/linq/linq_tests.h | 1 + 5 files changed, 116 insertions(+), 63 deletions(-) diff --git a/lib/coveo/linq/detail/linq_detail.h b/lib/coveo/linq/detail/linq_detail.h index 767b0e3..0132332 100644 --- a/lib/coveo/linq/detail/linq_detail.h +++ b/lib/coveo/linq/detail/linq_detail.h @@ -722,7 +722,7 @@ class element_at_impl private: // If we have random-access iterators, we can perform fast computations template - auto impl(Seq&& seq, std::random_access_iterator_tag) -> decltype(*std::begin(seq)) { + auto impl(Seq&& seq, std::random_access_iterator_tag) -> typename seq_traits::raw_value_type { auto icur = std::begin(seq); auto iend = std::end(seq); if (static_cast(iend - icur) <= n_) { @@ -734,7 +734,7 @@ class element_at_impl // Otherwise, we can only move by hand template - auto impl(Seq&& seq, std::input_iterator_tag) -> decltype(*std::begin(seq)) { + auto impl(Seq&& seq, std::input_iterator_tag) -> typename seq_traits::raw_value_type { auto icur = std::begin(seq); auto iend = std::end(seq); for (std::size_t i = 0; i < n_ && icur != iend; ++i, ++icur) { @@ -750,7 +750,7 @@ class element_at_impl : n_(n) { } template - auto operator()(Seq&& seq) -> decltype(*std::begin(seq)) { + auto operator()(Seq&& seq) -> typename seq_traits::raw_value_type { return impl(std::forward(seq), typename std::iterator_traits::iterator_type>::iterator_category()); } @@ -925,7 +925,7 @@ class first_impl_0 { public: template - auto operator()(Seq&& seq) -> decltype(*std::begin(seq)) { + auto operator()(Seq&& seq) -> typename seq_traits::raw_value_type { auto icur = std::begin(seq); if (icur == std::end(seq)) { throw_linq_empty_sequence(); @@ -946,7 +946,7 @@ class first_impl_1 : pred_(pred) { } template - auto operator()(Seq&& seq) -> decltype(*std::begin(seq)) { + auto operator()(Seq&& seq) -> typename seq_traits::raw_value_type { auto icur = std::begin(seq); auto iend = std::end(seq); if (icur == iend) { @@ -1638,7 +1638,7 @@ class last_impl_0 private: // If we have bidi iterators, we can simply use rbegin template - auto impl(Seq&& seq, std::bidirectional_iterator_tag) -> decltype(*std::begin(seq)) { + auto impl(Seq&& seq, std::bidirectional_iterator_tag) -> typename seq_traits::raw_value_type { auto ricur = seq.rbegin(); if (ricur == seq.rend()) { throw_linq_empty_sequence(); @@ -1648,7 +1648,7 @@ class last_impl_0 // Otherwise we'll have to be creative template - auto impl(Seq&& seq, std::input_iterator_tag) -> decltype(*std::begin(seq)) { + auto impl(Seq&& seq, std::input_iterator_tag) -> typename seq_traits::raw_value_type { auto icur = std::begin(seq); auto iend = std::end(seq); if (icur == iend) { @@ -1664,7 +1664,7 @@ class last_impl_0 public: template - auto operator()(Seq&& seq) -> decltype(*std::begin(seq)) { + auto operator()(Seq&& seq) -> typename seq_traits::raw_value_type { return impl(std::forward(seq), typename std::iterator_traits::iterator_type>::iterator_category()); } @@ -1680,7 +1680,7 @@ class last_impl_1 private: // If we have bidi iterators, we can simply use rbegin template - auto impl(Seq&& seq, std::bidirectional_iterator_tag) -> decltype(*std::begin(seq)) { + auto impl(Seq&& seq, std::bidirectional_iterator_tag) -> typename seq_traits::raw_value_type { auto ricur = seq.rbegin(); auto riend = seq.rend(); if (ricur == riend) { @@ -1695,7 +1695,7 @@ class last_impl_1 // Otherwise we'll have to be creative template - auto impl(Seq&& seq, std::input_iterator_tag) -> decltype(*std::begin(seq)) { + auto impl(Seq&& seq, std::input_iterator_tag) -> typename seq_traits::raw_value_type { auto icur = std::begin(seq); auto iend = std::end(seq); if (icur == iend) { @@ -1719,7 +1719,7 @@ class last_impl_1 : pred_(pred) { } template - auto operator()(Seq&& seq) -> decltype(*std::begin(seq)) { + auto operator()(Seq&& seq) -> typename seq_traits::raw_value_type { return impl(std::forward(seq), typename std::iterator_traits::iterator_type>::iterator_category()); } @@ -1810,7 +1810,7 @@ class max_impl_0 { public: template - auto operator()(Seq&& seq) -> decltype(*std::begin(seq)) { + auto operator()(Seq&& seq) -> typename seq_traits::raw_value_type { auto iend = std::end(seq); auto imax = std::max_element(std::begin(seq), iend); if (imax == iend) { @@ -1852,7 +1852,7 @@ class min_impl_0 { public: template - auto operator()(Seq&& seq) -> decltype(*std::begin(seq)) { + auto operator()(Seq&& seq) -> typename seq_traits::raw_value_type { auto iend = std::end(seq); auto imin = std::min_element(std::begin(seq), iend); if (imin == iend) { @@ -2333,7 +2333,7 @@ class single_impl_0 { public: template - auto operator()(Seq&& seq) -> decltype(*std::begin(seq)) { + auto operator()(Seq&& seq) -> typename seq_traits::raw_value_type { auto ifirst = std::begin(seq); auto iend = std::end(seq); if (ifirst == iend) { @@ -2360,7 +2360,7 @@ class single_impl_1 : pred_(pred) { } template - auto operator()(Seq&& seq) -> decltype(*std::begin(seq)) { + auto operator()(Seq&& seq) -> typename seq_traits::raw_value_type { auto ibeg = std::begin(seq); auto iend = std::end(seq); if (ibeg == iend) { diff --git a/lib/coveo/linq/linq.h b/lib/coveo/linq/linq.h index c796585..53dd638 100644 --- a/lib/coveo/linq/linq.h +++ b/lib/coveo/linq/linq.h @@ -52,7 +52,7 @@ auto from_range(ItBeg&& ibeg, ItEnd&& iend) // of integer values. Use like this: // // using namespace coveo::linq; -// auto result = from_int_range(1, 10) +// auto result = from_int_range(1, 10) // 1, 2, 3... // | linq_operator(...) // | ...; // @@ -73,7 +73,7 @@ auto from_int_range(IntT first, std::size_t count) // of repeated values. Use like this: // // using namespace coveo::linq; -// auto result = from_repeated(std::string("Life"), 7) +// auto result = from_repeated(std::string("Life"), 7) // "Life", "Life", "Life"... // | linq_operator(...) // | ...; // diff --git a/tests/coveo/linq/all_tests.cpp b/tests/coveo/linq/all_tests.cpp index e7045ac..3dd8f1b 100644 --- a/tests/coveo/linq/all_tests.cpp +++ b/tests/coveo/linq/all_tests.cpp @@ -19,6 +19,7 @@ void all_tests() // linq linq_tests(); chaining_tests(); + dangling_ref_tests(); } // Runs all benchmarks for coveo::enumerable and coveo::linq diff --git a/tests/coveo/linq/linq_tests.cpp b/tests/coveo/linq/linq_tests.cpp index bff0146..1d5d058 100644 --- a/tests/coveo/linq/linq_tests.cpp +++ b/tests/coveo/linq/linq_tests.cpp @@ -347,13 +347,6 @@ void linq_tests() COVEO_ASSERT((from(v) | element_at(1)) == 23); COVEO_ASSERT_THROW(from(v) | element_at(3)); } - { - std::vector v = { 42, 23, 66 }; - - using namespace coveo::linq; - (from(v) | element_at(1)) *= 2; - COVEO_ASSERT(v[1] == 46); - } // element_at_or_default { @@ -403,16 +396,6 @@ void linq_tests() COVEO_ASSERT((from(v) | first([](int i) { return i % 2 != 0; })) == 23); } - { - std::vector v = { 42, 23, 66 }; - const std::vector expected = { 43, 22, 66 }; - - using namespace coveo::linq; - (from(v) | first([](int i) { return i % 2 != 0; })) -= 1; - (from(v) | first()) += 1; - COVEO_ASSERT(detail::equal(std::begin(v), std::end(v), - std::begin(expected), std::end(expected))); - } // first_or_default { @@ -779,16 +762,6 @@ void linq_tests() COVEO_ASSERT((from(v) | last([](int i) { return i % 2 != 0; })) == 11); } - { - std::vector v = { 42, 23, 66, 11, 24 }; - const std::vector expected = { 42, 23, 66, 10, 25 }; - - using namespace coveo::linq; - (from(v) | last([](int i) { return i % 2 != 0; })) -= 1; - (from(v) | last()) += 1; - COVEO_ASSERT(detail::equal(std::begin(v), std::end(v), - std::begin(expected), std::end(expected))); - } // last_or_default { @@ -1077,25 +1050,6 @@ void linq_tests() COVEO_ASSERT((from(one_42_v) | single(equal_to_42)) == 42); COVEO_ASSERT_THROW(from(two_42_v) | single(equal_to_42)); } - { - std::vector v = { 42 }; - const std::vector expected = { 43 }; - - using namespace coveo::linq; - (from(v) | single()) += 1; - COVEO_ASSERT(detail::equal(std::begin(v), std::end(v), - std::begin(expected), std::end(expected))); - } - { - std::vector v = { 23, 42, 66 }; - const std::vector expected = { 23, 84, 66 }; - auto equal_to_42 = std::bind(std::equal_to(), std::placeholders::_1, 42); - - using namespace coveo::linq; - (from(v) | single(equal_to_42)) *= 2; - COVEO_ASSERT(detail::equal(std::begin(v), std::end(v), - std::begin(expected), std::end(expected))); - } // single_or_default { @@ -1661,6 +1615,103 @@ void chaining_tests() #endif } +// Runs tests for possible dangling references +void dangling_ref_tests() +{ + // Some LINQ operators used to return references to sequence elements. + // This is not compatible with sequences built around enumerable and + // temporaries stored in unique_ptrs though, so this had to be changed. + + struct foo { + std::string s_; + int i_; + foo(const std::string& s, int i) + : s_(s), i_(i) { } + }; + const std::vector vfoo = {{ "Life", 42 }, { "Hangar", 23 }, { "Route", 66 }}; + const std::vector vonefoo = {{ "Life", 42 }}; + + using namespace coveo::linq; + + // element_at + { + auto v = from(vfoo) + | select([](const foo& f) { return f.i_; }) + | element_at(1); + COVEO_ASSERT(v == 23); + } + + // first + { + auto v = from(vfoo) + | select([](const foo& f) { return f.i_; }) + | first(); + COVEO_ASSERT(v == 42); + } + { + auto v = from(vfoo) + | select([](const foo& f) { return f.i_; }) + | first([](int i) { return i % 2 != 0; }); + COVEO_ASSERT(v == 23); + } + + // last + { + auto v = from(vfoo) + | select([](const foo& f) { return f.i_; }) + | last(); + COVEO_ASSERT(v == 66); + } + { + auto v = from(vfoo) + | select([](const foo& f) { return f.i_; }) + | last([](int i) { return i % 2 != 0; }); + COVEO_ASSERT(v == 23); + } + + // max + { + auto v = from(vfoo) + | select([](const foo& f) { return f.i_; }) + | max(); + COVEO_ASSERT(v == 66); + } + { + auto v = from(vfoo) + | select([](const foo& f) { return f.i_; }) + | max([](int i) { return i * 2; }); + COVEO_ASSERT(v == 132); + } + + // min + { + auto v = from(vfoo) + | select([](const foo& f) { return f.i_; }) + | min(); + COVEO_ASSERT(v == 23); + } + { + auto v = from(vfoo) + | select([](const foo& f) { return f.i_; }) + | min([](int i) { return i * 2; }); + COVEO_ASSERT(v == 46); + } + + // single + { + auto v = from(vonefoo) + | select([](const foo& f) { return f.i_; }) + | single(); + COVEO_ASSERT(v == 42); + } + { + auto v = from(vfoo) + | select([](const foo& f) { return f.i_; }) + | single([](int i) { return i % 2 != 0; }); + COVEO_ASSERT(v == 23); + } +} + // Runs all benchmarks for coveo::linq operators void linq_benchmarks() { diff --git a/tests/coveo/linq/linq_tests.h b/tests/coveo/linq/linq_tests.h index d863141..dbbc5e0 100644 --- a/tests/coveo/linq/linq_tests.h +++ b/tests/coveo/linq/linq_tests.h @@ -9,6 +9,7 @@ namespace linq { void linq_tests(); void chaining_tests(); +void dangling_ref_tests(); void linq_benchmarks();