Skip to content
This repository has been archived by the owner on Jan 19, 2024. It is now read-only.

Commit

Permalink
Fix dangling reference issues
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@clechasseur
clechasseur committed Nov 21, 2017
1 parent cc5fa0b commit 28f2282
Show file tree
Hide file tree
Showing 5 changed files with 116 additions and 63 deletions.
30 changes: 15 additions & 15 deletions lib/coveo/linq/detail/linq_detail.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -722,7 +722,7 @@ class element_at_impl
private:
// If we have random-access iterators, we can perform fast computations
template<typename Seq>
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<Seq>::raw_value_type {
auto icur = std::begin(seq);
auto iend = std::end(seq);
if (static_cast<std::size_t>(iend - icur) <= n_) {
Expand All @@ -734,7 +734,7 @@ class element_at_impl

// Otherwise, we can only move by hand
template<typename Seq>
auto impl(Seq&& seq, std::input_iterator_tag) -> decltype(*std::begin(seq)) {
auto impl(Seq&& seq, std::input_iterator_tag) -> typename seq_traits<Seq>::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) {
Expand All @@ -750,7 +750,7 @@ class element_at_impl
: n_(n) { }

template<typename Seq>
auto operator()(Seq&& seq) -> decltype(*std::begin(seq)) {
auto operator()(Seq&& seq) -> typename seq_traits<Seq>::raw_value_type {
return impl(std::forward<Seq>(seq),
typename std::iterator_traits<typename seq_traits<Seq>::iterator_type>::iterator_category());
}
Expand Down Expand Up @@ -925,7 +925,7 @@ class first_impl_0
{
public:
template<typename Seq>
auto operator()(Seq&& seq) -> decltype(*std::begin(seq)) {
auto operator()(Seq&& seq) -> typename seq_traits<Seq>::raw_value_type {
auto icur = std::begin(seq);
if (icur == std::end(seq)) {
throw_linq_empty_sequence();
Expand All @@ -946,7 +946,7 @@ class first_impl_1
: pred_(pred) { }

template<typename Seq>
auto operator()(Seq&& seq) -> decltype(*std::begin(seq)) {
auto operator()(Seq&& seq) -> typename seq_traits<Seq>::raw_value_type {
auto icur = std::begin(seq);
auto iend = std::end(seq);
if (icur == iend) {
Expand Down Expand Up @@ -1638,7 +1638,7 @@ class last_impl_0
private:
// If we have bidi iterators, we can simply use rbegin
template<typename Seq>
auto impl(Seq&& seq, std::bidirectional_iterator_tag) -> decltype(*std::begin(seq)) {
auto impl(Seq&& seq, std::bidirectional_iterator_tag) -> typename seq_traits<Seq>::raw_value_type {
auto ricur = seq.rbegin();
if (ricur == seq.rend()) {
throw_linq_empty_sequence();
Expand All @@ -1648,7 +1648,7 @@ class last_impl_0

// Otherwise we'll have to be creative
template<typename Seq>
auto impl(Seq&& seq, std::input_iterator_tag) -> decltype(*std::begin(seq)) {
auto impl(Seq&& seq, std::input_iterator_tag) -> typename seq_traits<Seq>::raw_value_type {
auto icur = std::begin(seq);
auto iend = std::end(seq);
if (icur == iend) {
Expand All @@ -1664,7 +1664,7 @@ class last_impl_0

public:
template<typename Seq>
auto operator()(Seq&& seq) -> decltype(*std::begin(seq)) {
auto operator()(Seq&& seq) -> typename seq_traits<Seq>::raw_value_type {
return impl(std::forward<Seq>(seq),
typename std::iterator_traits<typename seq_traits<Seq>::iterator_type>::iterator_category());
}
Expand All @@ -1680,7 +1680,7 @@ class last_impl_1
private:
// If we have bidi iterators, we can simply use rbegin
template<typename Seq>
auto impl(Seq&& seq, std::bidirectional_iterator_tag) -> decltype(*std::begin(seq)) {
auto impl(Seq&& seq, std::bidirectional_iterator_tag) -> typename seq_traits<Seq>::raw_value_type {
auto ricur = seq.rbegin();
auto riend = seq.rend();
if (ricur == riend) {
Expand All @@ -1695,7 +1695,7 @@ class last_impl_1

// Otherwise we'll have to be creative
template<typename Seq>
auto impl(Seq&& seq, std::input_iterator_tag) -> decltype(*std::begin(seq)) {
auto impl(Seq&& seq, std::input_iterator_tag) -> typename seq_traits<Seq>::raw_value_type {
auto icur = std::begin(seq);
auto iend = std::end(seq);
if (icur == iend) {
Expand All @@ -1719,7 +1719,7 @@ class last_impl_1
: pred_(pred) { }

template<typename Seq>
auto operator()(Seq&& seq) -> decltype(*std::begin(seq)) {
auto operator()(Seq&& seq) -> typename seq_traits<Seq>::raw_value_type {
return impl(std::forward<Seq>(seq),
typename std::iterator_traits<typename seq_traits<Seq>::iterator_type>::iterator_category());
}
Expand Down Expand Up @@ -1810,7 +1810,7 @@ class max_impl_0
{
public:
template<typename Seq>
auto operator()(Seq&& seq) -> decltype(*std::begin(seq)) {
auto operator()(Seq&& seq) -> typename seq_traits<Seq>::raw_value_type {
auto iend = std::end(seq);
auto imax = std::max_element(std::begin(seq), iend);
if (imax == iend) {
Expand Down Expand Up @@ -1852,7 +1852,7 @@ class min_impl_0
{
public:
template<typename Seq>
auto operator()(Seq&& seq) -> decltype(*std::begin(seq)) {
auto operator()(Seq&& seq) -> typename seq_traits<Seq>::raw_value_type {
auto iend = std::end(seq);
auto imin = std::min_element(std::begin(seq), iend);
if (imin == iend) {
Expand Down Expand Up @@ -2333,7 +2333,7 @@ class single_impl_0
{
public:
template<typename Seq>
auto operator()(Seq&& seq) -> decltype(*std::begin(seq)) {
auto operator()(Seq&& seq) -> typename seq_traits<Seq>::raw_value_type {
auto ifirst = std::begin(seq);
auto iend = std::end(seq);
if (ifirst == iend) {
Expand All @@ -2360,7 +2360,7 @@ class single_impl_1
: pred_(pred) { }

template<typename Seq>
auto operator()(Seq&& seq) -> decltype(*std::begin(seq)) {
auto operator()(Seq&& seq) -> typename seq_traits<Seq>::raw_value_type {
auto ibeg = std::begin(seq);
auto iend = std::end(seq);
if (ibeg == iend) {
Expand Down
4 changes: 2 additions & 2 deletions lib/coveo/linq/linq.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(...)
// | ...;
//
Expand All @@ -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(...)
// | ...;
//
Expand Down
1 change: 1 addition & 0 deletions tests/coveo/linq/all_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ void all_tests()
// linq
linq_tests();
chaining_tests();
dangling_ref_tests();
}

// Runs all benchmarks for coveo::enumerable and coveo::linq
Expand Down
143 changes: 97 additions & 46 deletions tests/coveo/linq/linq_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<int> v = { 42, 23, 66 };

using namespace coveo::linq;
(from(v) | element_at(1)) *= 2;
COVEO_ASSERT(v[1] == 46);
}

// element_at_or_default
{
Expand Down Expand Up @@ -403,16 +396,6 @@ void linq_tests()
COVEO_ASSERT((from(v)
| first([](int i) { return i % 2 != 0; })) == 23);
}
{
std::vector<int> v = { 42, 23, 66 };
const std::vector<int> 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
{
Expand Down Expand Up @@ -779,16 +762,6 @@ void linq_tests()
COVEO_ASSERT((from(v)
| last([](int i) { return i % 2 != 0; })) == 11);
}
{
std::vector<int> v = { 42, 23, 66, 11, 24 };
const std::vector<int> 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
{
Expand Down Expand Up @@ -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<int> v = { 42 };
const std::vector<int> 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<int> v = { 23, 42, 66 };
const std::vector<int> expected = { 23, 84, 66 };
auto equal_to_42 = std::bind(std::equal_to<int>(), 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
{
Expand Down Expand Up @@ -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<foo> vfoo = {{ "Life", 42 }, { "Hangar", 23 }, { "Route", 66 }};
const std::vector<foo> 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()
{
Expand Down
1 change: 1 addition & 0 deletions tests/coveo/linq/linq_tests.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ namespace linq {

void linq_tests();
void chaining_tests();
void dangling_ref_tests();

void linq_benchmarks();

Expand Down

0 comments on commit 28f2282

Please sign in to comment.