use auto instead of iterator types, and related #1129

barendgehrels · 2023-04-10T18:50:58Z

It's a big PR but quite mechanical

barendgehrels · 2023-04-10T18:52:12Z

include/boost/geometry/algorithms/centroid.hpp

-                it = boost::begin(multi);
-            it != boost::end(multi);
-            ++it)
+        for (auto it = boost::begin(multi); it != boost::end(multi); ++it)


NOTE: we cannot do a range based for loop here
(as @awulkiew pointed out once).

In some places I changed it to a range based for loop, but these places are internal (for example turns).

awulkiew · 2023-04-10T20:36:02Z

include/boost/geometry/algorithms/densify.hpp

-            point_t const& p0 = *prev;
-            point_t const& p1 = *it;
+            auto const& p0 = *prev;
+            auto const& p1 = *it;


Note that here iterator could return non-true reference type. Previously this type was converted to point_t which was value_type vs reference_type returned by iterator. If this non-true reference type was not adapted to Point concept the call to convert below could fail to compile. I don't know if that's a problem.

Thanks for your comments! I'll process them soon

I'm not sure either. Reverted (it was not related to the iterator anyway)
✔️

awulkiew · 2023-04-10T20:39:19Z

include/boost/geometry/algorithms/detail/buffer/buffered_piece_collection.hpp

@@ -791,7 +775,7 @@ struct buffered_piece_collection
    {
        BOOST_GEOMETRY_ASSERT(boost::size(range) != 0u);

-        typename Range::const_iterator it = boost::begin(range);
+        auto it = std::cbegin(range);


To double-check, is std intended here? boost::size is called above.

Reverted. First I changed some begin but later decided it's better to limit this to iterators only. This was a left over.
✔️

Unless there are objections, a separate PR could change boost::begin to std::begin everywhere (+end)

I did some more checks, we can't change it everywhere. Because we support geometries adapted to Boost.Range, but not std::begin etc.
We might (ever) change that, but that might break behavior for users having custom geometries.

awulkiew · 2023-04-10T20:42:26Z

include/boost/geometry/algorithms/detail/buffer/buffered_piece_collection.hpp

            // Update member used
-            turn.turn_index = index;
+            turn.turn_index = index++;


for_each_with_index is used below. Would it make sense to use it here as well?

It's currently a const version only.
But I can add an overload.

awulkiew · 2023-04-10T20:44:19Z

include/boost/geometry/algorithms/detail/calculate_point_order.hpp

@@ -36,7 +36,7 @@ struct clean_point
        , m_is_azi_valid(false), m_is_azi_diff_valid(false)
    {}

-    typename boost::iterators::iterator_reference<Iter>::type ref() const
+    auto ref() const


This way the reference will be converted to value. The function should probably return decltype(auto).

✔️ reverted

FYI, revert was not required. Returning decltype(auto) would be enough.

✔️ intuitively it looked the same, but I get it now. Changed. Thank you.

awulkiew · 2023-04-10T20:46:53Z

include/boost/geometry/algorithms/detail/counting.hpp

-                 it = boost::begin(geometry);
-             it != boost::end(geometry);
-             ++it)
+        for (auto const& geometry : multi)


Range-based for loop is probably incorrect here.

✔️ right, reverted

awulkiew · 2023-04-11T08:27:59Z

include/boost/geometry/algorithms/detail/envelope/range_of_boxes.hpp

-        for (iterator_type it = boost::begin(range_of_boxes);
-             it != boost::end(range_of_boxes);
-             ++it)
+        for (auto const& box : range_of_boxes)


While it's true that currently this function is used only internally I wouldn't be so eager to replace this loop with range-based version. It's because technically we could introduce MultiBox concept in the future. In general I propose to leave normal loops in algorithms taking ranges as template parameters as if they were geometries because replacing them may have unintended consequences for users.

✔️ good point, reverted (keeping box)

awulkiew · 2023-04-11T08:33:51Z

include/boost/geometry/algorithms/detail/is_valid/has_spikes.hpp

        typename boost::range_reverse_iterator
            <
                View const
            >::type prev = find_different_from_first(boost::rbegin(view),
                                                     boost::rend(view),
                                                     strategy);

-        iterator next = find_different_from_first(cur, boost::end(view),
-                                                  strategy);
+        auto next = find_different_from_first(cur, boost::end(view), strategy);


We're loosing information here. Previously a reader was able to see that next was indeed an iterator. However now we don't know what it is. It might be a point or an iterator or something else. I'm thinking about a rule to always have it in the name for auto iterators. So cur_it and next_it. Does it make sense?
EDIT: btw, I'm not sure if that's a good idea. ;)

We often use next. The code currently does not use next_it.
I kept it here.
But I missed the one on line 80/78 earlier, that's done now ✔️

We have it_min though. So it_next would work. But I didn't change it, we can consider what we should do in general.

awulkiew · 2023-04-11T08:36:35Z

include/boost/geometry/algorithms/detail/num_distinct_consecutive_points.hpp

        std::size_t counter(0);
        do
        {
            ++counter;
-            iterator next = std::find_if(current, end, [&](auto const& pt) {
+            auto next = std::find_if(current, end, [&](auto const& pt) {


Though here the names doesn't bother me because I can see what they are from the context.

awulkiew · 2023-04-11T08:40:46Z

include/boost/geometry/algorithms/detail/overlay/assign_parents.hpp

-                    geometry::envelope(get_ring<tag1>::apply(it->first, geometry1),
-                                       item.envelope, strategy);
+                    geometry::envelope(get_ring<tag1>::apply(pair.first, geometry1),
+                                    item.envelope, strategy);


The indentation was changed. Was it intentional?

I see, no it was not intentional. ✔️

awulkiew · 2023-04-11T08:44:01Z

include/boost/geometry/algorithms/detail/overlay/assign_parents.hpp

                {
                    if (index != index_positive)
                    {
-                        ring_info_type& inner = ring_map[it->id];
+                        ring_info_type& inner = ring_map[item.id];


Out of curiosity, why have you left ring_info_type? Not that I'm against it becasue I think that auto should be used carefully and only if it improves readability. I'm just curious.

Indeed using auto is delicate. For iterators it's perfect. If it saves type definitions, I'm also inclined to use it.
In this case it would not save that type.

awulkiew · 2023-04-11T08:53:26Z

include/boost/geometry/algorithms/detail/overlay/intersection_insert.hpp

+        for_each_with_index(turns, [](auto index, auto const& turn)
+            {
+                debug_follow(turn, turn.operations[0], index);
+            });


The indentation is inconsistent with other calls. Besides we should probably mimic loops as it is correctly done in other places.

awulkiew · 2023-04-11T09:02:30Z

include/boost/geometry/algorithms/detail/partition.hpp

+        for (auto it = boost::begin(forward_range);
            it != boost::end(forward_range);
            ++it)


Would it make sense to put the loop in one line? It should be lesser than 100 characters.

Yes it fits.
✔️

awulkiew · 2023-04-11T09:04:28Z

include/boost/geometry/algorithms/detail/relate/linear_areal.hpp

@@ -305,7 +304,7 @@ inline bool calculate_from_inside(Geometry1 const& geometry1,
        std::size_t const q_seg_jk = (q_seg_ij + 1) % seg_count2;
        // TODO: the following function should return immediately, however the worst case complexity is O(N)
        // It would be good to replace it with some O(1) mechanism
-        range2_iterator qk_it = find_next_non_duplicated(boost::begin(range2),
+        auto qk_it = find_next_non_duplicated(boost::begin(range2),
                                                         range::pos(range2, q_seg_jk),


Indentation should probably be changed as well.

awulkiew · 2023-04-11T09:04:43Z

include/boost/geometry/algorithms/detail/relate/follow_helpers.hpp

        // search for the entry point in the same range of other geometry
-        point_iterator entry_it = std::find_if(m_other_entry_points.begin(),
+        auto entry_it = std::find_if(m_other_entry_points.begin(),
                                               m_other_entry_points.end(),


Indentation should probably be changed as well.

awulkiew · 2023-04-11T09:06:58Z

include/boost/geometry/algorithms/detail/relate/point_point.hpp

-    for ( ; it != last ; ++it )
+    auto it = boost::begin(multi_point);
+    auto last = boost::end(multi_point);
+    for ( ; it != last ; ++it)


These iterators could probably be put inside the loop. At least it because with last/end there could be a debate about performance.

✔️ fixed and renamed last to end as done elsewhere

awulkiew · 2023-04-11T09:10:11Z

include/boost/geometry/algorithms/detail/relate/topology_check.hpp

        {
-            typename boost::range_reference<MultiLinestring const>::type
-                ls = *it;
+            auto ls = *it;


This would cause a linestring to be copied. auto const& should probably be used here instead.

It's an issue preventing me to approve the PR.

👍 well spotted, thanks
✔️ changed to auto const& and a few lines later (for points) as well.

awulkiew · 2023-04-11T09:33:44Z

include/boost/geometry/algorithms/detail/within/within_no_turns.hpp

-        for ( iterator it = boost::begin(geometry::interior_rings(geometry2)) ;
-              it != boost::end(geometry::interior_rings(geometry2)) ;
-              ++it )
+        auto&& rings2 = geometry::interior_rings(geometry2);


AFAIU the intention is to represent const reference (even if non-true temporary), not forwarding/universal reference so auto const& would probably be more appropriate.

I copied this from https://github.com/boostorg/geometry/blob/develop/include/boost/geometry/algorithms/correct.hpp#L135
but indeed, it's not const there.

Changed to auto const&, indeed we use that more often

awulkiew · 2023-04-11T09:34:01Z

include/boost/geometry/algorithms/detail/within/within_no_turns.hpp

-        for ( iterator2 it = boost::begin(geometry::interior_rings(geometry2)) ;
-              it != boost::end(geometry::interior_rings(geometry2)) ;
-              ++it )
+        auto&& rings2 = geometry::interior_rings(geometry2);


✔️ fixed

awulkiew · 2023-04-11T09:34:36Z

include/boost/geometry/algorithms/detail/within/within_no_turns.hpp

-                for ( iterator1 it1 = boost::begin(geometry::interior_rings(geometry1)) ;
-                      it1 != boost::end(geometry::interior_rings(geometry1)) ;
-                      ++it1 )
+                auto&& rings1 = geometry::interior_rings(geometry1);


✔️ fixed

awulkiew · 2023-04-11T09:37:06Z

include/boost/geometry/algorithms/for_each.hpp

-        typename interior_return_type<Polygon>::type
-            rings = interior_rings(poly);
-
+        auto&& rings = interior_rings(poly);


Forwarding reference, are we planning to forward rings?

Here it is a non-const.
So I changed it like done in correct.
Maybe auto& is better in this case, changed it to that. I think that's OK here, @awulkiew ?

Right, if it's non-const then we have to use auto&&. If we use auto& the lifetime of a temporary which might be returned by interior_rings() will not be extended.

awulkiew · 2023-04-11T09:39:33Z

include/boost/geometry/io/svg/write.hpp

-                rings = interior_rings(polygon);
-            for (typename detail::interior_iterator<Polygon const>::type
-                    rit = boost::begin(rings); rit != boost::end(rings); ++rit)
+            auto&& rings = interior_rings(polygon);


Forwarding reference.

✔️ fixed

awulkiew · 2023-04-11T09:40:51Z

include/boost/geometry/algorithms/centroid.hpp

-
-        for (typename detail::interior_iterator<Polygon const>::type
-                it = boost::begin(rings); it != boost::end(rings); ++it)
+        auto&& rings = interior_rings(poly);


The intention here is to represent const reference, not forwarding reference. auto const& should probably be used here instead.

✔️ fixed

awulkiew

That's a big PR. Thanks!
I see some minor issues and one preventing me to approve it, i.e. unintended copy of a linestring. There is also unintended copy of a point but that's not that big of a problem.

vissarion

Thanks, LGTM in general with some (minor) comments.

vissarion · 2023-04-11T09:13:46Z

include/boost/geometry/algorithms/centroid.hpp

-
-        for (typename detail::interior_iterator<Polygon const>::type
-                it = boost::begin(rings); it != boost::end(rings); ++it)
+        auto&& rings = interior_rings(poly);


Should we be more specific here using auto const & since rings are not modifiable?

👍 ✔️ fixed

vissarion · 2023-04-11T09:15:09Z

include/boost/geometry/algorithms/centroid.hpp

-        for (typename detail::interior_iterator<Polygon const>::type
-                it = boost::begin(rings); it != boost::end(rings); ++it)
+        auto&& rings = interior_rings(poly);
+        auto const end = boost::end(rings);


What is the reason of copying here and not just using it as boost::end(rings) inside for loop as you are doing below?

It's done elsewhere as well, for rings.

I'm not sure if this is the reason but the effect is that end will not be calculated at the end of each iteration.

vissarion · 2023-04-11T09:16:34Z

include/boost/geometry/algorithms/densify.hpp

-        iterator_t it = boost::begin(rng);
-        iterator_t end = boost::end(rng);
+        auto it = boost::begin(rng);
+        auto end = boost::end(rng);


Is it better to declare it as auto const or even auto const& ?

~~You see it occasionally, but usually iterators are never declared const.~~
I would not make them auto const& because it's not a reference (though it's syntactically correct)

Edit: fixed, made const ✔️

vissarion · 2023-04-11T09:36:24Z

include/boost/geometry/algorithms/detail/buffer/buffered_piece_collection.hpp

@@ -801,7 +785,7 @@ struct buffered_piece_collection
            add_point(*it);
        }

-        for (++it; it != boost::end(range); ++it)
+        for (++it; it != std::cend(range); ++it)


Similar to @awulkiew's comment above on std::cbegin. Is there an intention to remove dependencies on boost::begin and boost::end from this file? If this is the case I think you could also remove the inclusion of the related header files in the beginning of this file.

👍 ✔️ reverted

vissarion · 2023-04-11T09:46:26Z

include/boost/geometry/algorithms/detail/distance/geometry_to_segment_or_box.hpp

-        seg_or_box_const_iterator it_min1 = boost::const_begin(seg_or_box_points);
-        seg_or_box_const_iterator it_min2 = it_min1;
+        auto pit_min = points_begin(geometry);
+        auto it_min1 = boost::const_begin(seg_or_box_points);


shouldn't this and the next one be auto const ?

~~See above, we don't have that habit for iterators~~

~~Edit: fixed~~

Edit2: rolled back, it was actually changed

Defining it as auto const will prevent us from changing the iterator e.g incrementg it. If that's ok then it could be const like in case of end/last. Think about a pointer to const (const_iterator) vs const pointer (auto const).

Yes, I changed it where @vissarion requested this. I completely agree now.

But specifically in this case, this iterator is meant to change. It is an iterator pointing to the minimum. So it cannot be made const here.

vissarion · 2023-04-11T09:50:24Z

include/boost/geometry/algorithms/detail/distance/geometry_to_segment_or_box.hpp

@@ -318,7 +301,7 @@ class geometry_to_segment_or_box
    {
        distance::creturn_t<MultiPoint, SegmentOrBox, Strategies> cd_min;

-        iterator_type it_min
+        auto it_min


should it be auto const ?

~~Same here~~
edit: fixed ✔️

vissarion · 2023-04-11T09:51:38Z

include/boost/geometry/algorithms/detail/envelope/range_of_boxes.hpp

-        iterator_type it_max = std::max_element(boost::begin(range_of_boxes),
-                                                boost::end(range_of_boxes),
-                                                latitude_less<max_corner>());
+        auto it_min = std::min_element(boost::begin(range_of_boxes),


auto const?

~~Same here~~
edit: fixed ✔️

vissarion · 2023-04-11T10:07:21Z

include/boost/geometry/algorithms/line_interpolate.hpp

-        iterator_t it = boost::begin(range);
-        iterator_t end = boost::end(range);
+        auto it = boost::begin(range);
+        auto end = boost::end(range);


auto const ?

~~Same here~~
Edit: for end which is never changed, we actually did it more often. Changed here ✔️

barendgehrels · 2023-04-12T12:57:52Z

New version is pushed.
Thanks for the thorough reviews!

I'm still doing some checks, adding a unit test which catches those changes in range-based-for-loops (because apparently they did not exist yet for several algorithms), and catches copies.
So it does not need to be merged yet.

awulkiew · 2023-04-14T12:56:05Z

include/boost/geometry/algorithms/for_each.hpp

-        typename interior_return_type<Polygon>::type
-            rings = interior_rings(poly);
-
+        auto& rings = interior_rings(poly);


Suggested change

auto& rings = interior_rings(poly);

auto&& rings = interior_rings(poly);

✔️ fixed, thanks

awulkiew · 2023-04-14T13:04:14Z

include/boost/geometry/util/for_each_with_index.hpp

+    std::size_t index = 0;
+    for (auto it = std::begin(container); it != std::end(container); ++it, ++index)


Technically the type of index should be taken from Container/Range, e.g.: boost::size_type<Container>::type but I guess in practice std::size_t will be ok.

Are we sure that this utility will not be used for geometries adapted to Boost.Range Concept?
Would it make sense to use boost::begin and boost::end to be safe?

✔️ fixed. While fixing it, I now also use Boost Range's size type

barendgehrels · 2023-04-16T12:51:23Z

Results are pushed again. The last push also changes some other interior_ring_type occurrences, fixes a bug (concept) and adds two missing include files.
This is caught by the new unit test (still to be pushed).

barendgehrels · 2023-04-16T17:15:28Z

Second commit adds tests (and fixes related concept).
Finally I'll have to add similar tests for (multi)linestring.
It proves to be quite useful, several functions do not support these geometries

barendgehrels · 2023-04-16T17:16:21Z

include/boost/geometry/io/wkt/write.hpp

-        auto const exterior = exterior_ring(poly);
-        auto const rings = interior_rings(poly);
+        auto const& exterior = exterior_ring(poly);
+        auto const& rings = interior_rings(poly);


My bad, I changed this last December. We didn't have unit tests catching this.

barendgehrels · 2023-04-16T17:24:18Z

test/geometries/custom_non_copiable/cnc_container.hpp

+
+// Define a custom class which is only movable and not copiable, and cannot be indexed.
+// Its derivatives will be linestring, ring, multi*
+// For readability purposes we use the "cnc" abbreviation as "custom_non_copiable"


This class originates from all_custom_geometries but takes it a bit further, making it non-copyable.
For test purposes it has to be indexable, but that's also customized (the library does not see that).

Also the adaptations are more modern (requiring less typedefs, more readable)

awulkiew · 2023-04-18T20:26:00Z

Thanks!

barendgehrels · 2023-04-19T17:32:28Z

@vissarion many tests are added, I'll assume you are still OK. I will merge one of the coming days.

barendgehrels requested review from awulkiew and vissarion April 10, 2023 18:50

barendgehrels self-assigned this Apr 10, 2023

barendgehrels commented Apr 10, 2023

View reviewed changes

awulkiew reviewed Apr 10, 2023

View reviewed changes

awulkiew reviewed Apr 11, 2023

View reviewed changes

awulkiew requested changes Apr 11, 2023

View reviewed changes

vissarion approved these changes Apr 11, 2023

View reviewed changes

barendgehrels force-pushed the avoid-iterators-typedefs branch from ca00897 to cb3bc95 Compare April 12, 2023 12:56

awulkiew reviewed Apr 14, 2023

View reviewed changes

barendgehrels force-pushed the avoid-iterators-typedefs branch from cb3bc95 to d0bbb2f Compare April 16, 2023 12:50

barendgehrels commented Apr 16, 2023

View reviewed changes

awulkiew approved these changes Apr 18, 2023

View reviewed changes

barendgehrels force-pushed the avoid-iterators-typedefs branch 2 times, most recently from d2f37ca to 1dda97c Compare April 19, 2023 17:30

barendgehrels added 2 commits April 19, 2023 19:35

use auto instead of iterator types, and related

004980c

[test] add custom, non copiable, non indexable geometries

1e04cc6

barendgehrels force-pushed the avoid-iterators-typedefs branch from 1dda97c to 1e04cc6 Compare April 19, 2023 17:35

barendgehrels merged commit ee83f57 into boostorg:develop Apr 23, 2023

barendgehrels deleted the avoid-iterators-typedefs branch April 23, 2023 11:31

vissarion added this to the 1.83 milestone Jun 16, 2023

	auto& rings = interior_rings(poly);
	auto&& rings = interior_rings(poly);

		std::size_t index = 0;
		for (auto it = std::begin(container); it != std::end(container); ++it, ++index)

use auto instead of iterator types, and related #1129

use auto instead of iterator types, and related #1129

Conversation

barendgehrels commented Apr 10, 2023

Choose a reason for hiding this comment

awulkiew Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awulkiew Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awulkiew Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awulkiew Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

barendgehrels Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

awulkiew Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awulkiew Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awulkiew Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awulkiew Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awulkiew left a comment • edited Loading

Choose a reason for hiding this comment

vissarion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barendgehrels Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barendgehrels Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

awulkiew Apr 10, 2023 •

edited

Loading

awulkiew Apr 10, 2023 •

edited

Loading

awulkiew Apr 11, 2023 •

edited

Loading

awulkiew Apr 11, 2023 •

edited

Loading

barendgehrels Apr 12, 2023 •

edited

Loading

awulkiew Apr 11, 2023 •

edited

Loading

awulkiew Apr 11, 2023 •

edited

Loading

awulkiew Apr 11, 2023 •

edited

Loading

awulkiew Apr 11, 2023 •

edited

Loading

awulkiew left a comment •

edited

Loading

barendgehrels Apr 12, 2023 •

edited

Loading

barendgehrels Apr 12, 2023 •

edited

Loading

barendgehrels Apr 12, 2023 •

edited

Loading

barendgehrels Apr 12, 2023 •

edited

Loading

barendgehrels Apr 12, 2023 •

edited

Loading