Skip to content

Fix/closing iterator returns reference to temp #488

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 9, 2018
Merged

Fix/closing iterator returns reference to temp #488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
43538e5
[test][iterators] add test case that demonstrates SEGFAULT when using…
jonasdmentia Jun 7, 2018
1d7a38f
[iterators] fix return of a reference to a temporary object when usin…
jonasdmentia Jun 7, 2018
61679f4
[iterators] use the base type (iterator_facade) for the reference and…
jonasdmentia Jun 7, 2018
5a09648
[iterators] fix calculation of range's const reference type inside cl…
jonasdmentia Jul 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions include/boost/geometry/iterators/closing_iterator.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,24 @@ struct closing_iterator
<
closing_iterator<Range>,
typename boost::range_value<Range>::type const,
boost::random_access_traversal_tag
boost::random_access_traversal_tag,
typename boost::range_reference<Range const>::type,
typename boost::range_difference<Range>::type
>
{
typedef typename boost::range_difference<Range>::type difference_type;
private:
typedef boost::iterator_facade
<
closing_iterator<Range>,
typename boost::range_value<Range>::type const,
boost::random_access_traversal_tag,
typename boost::range_reference<Range const>::type,
typename boost::range_difference<Range>::type
> base_type;

public:
typedef typename base_type::reference reference;
typedef typename base_type::difference_type difference_type;

/// Constructor including the range it is based on
explicit inline closing_iterator(Range& range)
Expand Down Expand Up @@ -71,7 +85,7 @@ struct closing_iterator
private:
friend class boost::iterator_core_access;

inline typename boost::range_value<Range>::type const& dereference() const
inline reference dereference() const
{
return *m_iterator;
}
Expand Down
20 changes: 17 additions & 3 deletions include/boost/geometry/iterators/ever_circling_iterator.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,22 @@ struct ever_circling_range_iterator
<
ever_circling_range_iterator<Range>,
typename boost::range_value<Range>::type const,
boost::random_access_traversal_tag
boost::random_access_traversal_tag,
typename boost::range_reference<Range const>::type,
typename boost::range_difference<Range>::type
>
{
private:
typedef boost::iterator_facade
<
ever_circling_range_iterator<Range>,
typename boost::range_value<Range>::type const,
boost::random_access_traversal_tag,
typename boost::range_reference<Range const>::type,
typename boost::range_difference<Range>::type
> base_type;

public:
/// Constructor including the range it is based on
explicit inline ever_circling_range_iterator(Range& range)
: m_range(&range)
Expand All @@ -118,12 +131,13 @@ struct ever_circling_range_iterator
, m_index(0)
{}

typedef std::ptrdiff_t difference_type;
typedef typename base_type::reference reference;
typedef typename base_type::difference_type difference_type;

private:
friend class boost::iterator_core_access;

inline typename boost::range_value<Range>::type const& dereference() const
inline reference dereference() const
{
return *m_iterator;
}
Expand Down
33 changes: 33 additions & 0 deletions test/iterators/closing_iterator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
#include <boost/geometry/geometries/geometries.hpp>
#include <boost/geometry/geometries/point_xy.hpp>

#include <boost/range/adaptor/transformed.hpp>


// The closing iterator should also work on normal std:: containers
void test_empty_non_geometry()
Expand Down Expand Up @@ -73,6 +75,36 @@ void test_non_geometry()
BOOST_CHECK_EQUAL(out.str(), "1231");
}

void test_transformed_non_geometry()
{
std::vector<int> v;
v.push_back(-1);
v.push_back(-2);
v.push_back(-3);

typedef boost::transformed_range
<
std::negate<int>,
std::vector<int>
> transformed_range;

typedef bg::closing_iterator
<
transformed_range const
> closing_iterator;

transformed_range v2 = v | boost::adaptors::transformed(std::negate<int>());
closing_iterator it(v2);
closing_iterator end(v2, true);

std::ostringstream out;
for (; it != end; ++it)
{
out << *it;
}
BOOST_CHECK_EQUAL(out.str(), "1231");
}




Expand Down Expand Up @@ -129,6 +161,7 @@ void test_all()
{
test_empty_non_geometry();
test_non_geometry();
test_transformed_non_geometry();
test_geometry<bg::model::ring<P> >("POLYGON((1 1,1 4,4 4,4 1))");
}

Expand Down
54 changes: 50 additions & 4 deletions test/iterators/ever_circling_iterator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,14 @@
#include <boost/geometry/io/wkt/read.hpp>
#include <boost/geometry/geometries/geometries.hpp>
#include <boost/geometry/geometries/point_xy.hpp>
#include <boost/geometry/geometries/register/linestring.hpp>

#include <boost/range/adaptor/transformed.hpp>


template <typename G>
void test_geometry(std::string const& wkt)
void test_geometry(G const& geo)
{
G geo;
bg::read_wkt(wkt, geo);
typedef typename boost::range_iterator<G const>::type iterator_type;


Expand Down Expand Up @@ -74,7 +76,7 @@ void test_geometry(std::string const& wkt)
// Check the range_iterator-one
{
std::ostringstream out;
bg::ever_circling_range_iterator<G> it(geo);
bg::ever_circling_range_iterator<G const> it(geo);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you need to modify this line? I would expect the ever_circling range iterator to work with or with out the const.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ever_circling_range_iterator stores a non-const pointer to Range, but if the range you're constructing the iterator with is const then you end up trying to assign a pointer to a const range to the non-const pointer. I think ever_circling_range_iterator is doing the right thing here, it assumes the const-ness is captured as part of the Range template parameter, it's just that at the call site the G template parameter doesn't capture the const-ness and the variable geo is a 'G const&'....hence the explicit const.

This didn't matter before because geo was a local non-const variable, but now that I've separated the WKT parsing and pass geo in as 'G const&' it does. Note that the same thing was already being done in the iterator typedef used with ever_circling_iterator (i.e. the non-range version), I suspect it wasn't actually needed before for the same reason (geo was non-const) but it is now.

for (std::size_t i = 0; i < n; ++i, ++it)
{
out << bg::get<0>(*it);
Expand All @@ -83,10 +85,54 @@ void test_geometry(std::string const& wkt)
}
}

template <typename G>
void test_geometry(std::string const& wkt)
{
G geo;
bg::read_wkt(wkt, geo);
test_geometry(geo);
}


template <typename P>
P transform_point(P const& p)
{
P result;
bg::set<0>(result, bg::get<0>(p) + 1);
bg::set<1>(result, bg::get<1>(p) + 1);
return result;
}

template <typename G>
struct transformed_geometry_type
{
typedef typename bg::point_type<G>::type point_type;
typedef boost::transformed_range<point_type(*)(point_type const&), G> type;
};

template <typename G>
void test_transformed_geometry(G const& geo)
{
typedef typename bg::point_type<G>::type point_type;
test_geometry(geo | boost::adaptors::transformed(&transform_point<point_type>));
}

template <typename G>
void test_transformed_geometry(std::string const& wkt)
{
G geo;
bg::read_wkt(wkt, geo);
test_transformed_geometry(geo);
}


BOOST_GEOMETRY_REGISTER_LINESTRING(typename transformed_geometry_type< bg::model::linestring< bg::model::d2::point_xy<double> > >::type)

template <typename P>
void test_all()
{
test_geometry<bg::model::linestring<P> >("linestring(1 1,2 2,3 3,4 4,5 5)");
test_transformed_geometry<bg::model::linestring<P> >("linestring(0 0,1 1,2 2,3 3,4 4)");
}

int test_main(int, char* [])
Expand Down