[gdal-dev] Use of C++ iterators in API

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[gdal-dev] Use of C++ iterators in API

Even Rouault-2
Hi,

Just wanted to advertize the recent new introduction of C++ iterators over
GDALDataset (to iterate over its bands, layers and features), OGRLayer (to
iterate overs its features), OGRFeature (to iterate over its field values) and
OGRGeometry (if you iterate over a OGRGeometryCollection, you get its members.
If you iterate over a OGRPolygon, you get its rings, etc...)
Fields can also be get and set using the [] operator on a feature.

Taken from the updated API tutorial : http://gdal.org/ogr_apitut.html

#include "ogrsf_frmts.h"
int main()
{
    GDALAllRegister();
    GDALDatasetUniquePtr poDS(GDALDataset::Open( "point.shp",
GDAL_OF_VECTOR));
    if( poDS == nullptr )
    {
        printf( "Open failed.\n" );
        exit( 1 );
    }
    for( auto&& poLayer: poDS->GetLayers() )
    {
        for( auto&& poFeature: *poLayer )
        {
            for( auto&& oField: *poFeature )
            {
                switch( oField.GetType() )
                {
                    case OFTInteger:
                        printf( "%d,", oField.GetInteger() );
                        break;
                    case OFTInteger64:
                        printf( CPL_FRMT_GIB ",", oField.GetInteger64() );
                        break;
                    case OFTReal:
                        printf( "%.3f,", oField.GetDouble() );
                        break;
                    case OFTString:
                        printf( "%s,", oField.GetString() );
                        break;
                    default:
                        printf( "%s,", oField.GetAsString() );
                        break;
                }
          }
     }
}

There's also a visitor over OGRGeometry subclasses that can make life easier
( http://gdal.org/classOGRDefaultConstGeometryVisitor.html )

The autotest cpp stuff starting at
https://github.com/OSGeo/gdal/blob/master/autotest/cpp/test_ogr.cpp#L963
should get a good picture of all the new API.

Even


--
Spatialys - Geospatial professional services
http://www.spatialys.com
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Use of C++ iterators in API

Mateusz Loskot
On 10 April 2018 at 22:06, Even Rouault <[hidden email]> wrote:
> Hi,
>
> Just wanted to advertize the recent new introduction of C++ iterators (...)

Good one, thanks!

>     for( auto&& poLayer: poDS->GetLayers() )

Do the iterators return a proxy reference hence the tutorial prefers
auto&& over plain auto&?
(ie. like std::vector<bool> iterator does)

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Use of C++ iterators in API

Even Rouault-2
On mardi 10 avril 2018 22:34:56 CEST Mateusz Loskot wrote:

> On 10 April 2018 at 22:06, Even Rouault <[hidden email]> wrote:
> > Hi,
> >
> > Just wanted to advertize the recent new introduction of C++ iterators
> > (...)
>
> Good one, thanks!
>
> >     for( auto&& poLayer: poDS->GetLayers() )
>
> Do the iterators return a proxy reference hence the tutorial prefers
> auto&& over plain auto&?
> (ie. like std::vector<bool> iterator does)

Not completely sure to understand what a proxy reference is (C++11 stuff is
still very new to me, and I don't pretend to master C++98, so ...), but I feel
that must be close to what I have used in some cases, particularly wen
iterating over fields of a features, or points of a linestring.

I read in some tutorial that using auto&& worked in all situations, including
when the returned reference was const, so now I just use that everywhere.

In fact the nature of the object which is returned depends on the iterator,
and as documented in the API, for the sake of implementation simplicity, those
iterators do not necessarily respect the whole semantics of iterators.

That is if you do

auto iter = poLineString->begin();
OGRPoint& oPoint = *iter;
++iter;
OGRPoint& oAnotherPoint = *iter;
In fact &oAnotherPoint == &oPoint.

The reason is that internally a line string is not an array of OGRPoint, but
an array of OGRRawPoint + optional array of z + optional array of m, so the
iterator object only holds a single OGRPoint and always return the reference
to it when you dereference it, and updates its content when you increment it.

So basically you should only use them in range based loops, where you don't
really manipulate the iterator and cannot do stuff like the above.

I guess the "correct" solution would have been to return a OGRPointProxy that
would keep a link to the linestring and the point index. But not sure how you
would do that for a const iterator where the operator * is supposed to
returned a const reference... Except allocating a std::array<OGRPointProxy> in
the iterator, which seems overkill.

Improvements (if doable and not adding (too much) overhead to the underlying
objects) are welcome of course.

Even

--
Spatialys - Geospatial professional services
http://www.spatialys.com
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Use of C++ iterators in API

Mateusz Loskot
On 10 April 2018 at 23:06, Even Rouault <[hidden email]> wrote:

> On mardi 10 avril 2018 22:34:56 CEST Mateusz Loskot wrote:
>> On 10 April 2018 at 22:06, Even Rouault <[hidden email]> wrote:
>> >
>> > Just wanted to advertize the recent new introduction of C++ iterators
>> > (...)
>>
>> Good one, thanks!
>>
>> >     for( auto&& poLayer: poDS->GetLayers() )
>>
>> Do the iterators return a proxy reference hence the tutorial prefers
>> auto&& over plain auto&?
>> (ie. like std::vector<bool> iterator does)
>
> Not completely sure to understand what a proxy reference is (C++11 stuff is
> still very new to me, and I don't pretend to master C++98, so ...), but I feel
> that must be close to what I have used in some cases, particularly wen
> iterating over fields of a features, or points of a linestring.

I didn't look at the iterators impl. yet.
I just looked at your example and wondered if there are proxies
- for GDAL/OGR, those could be necessary, I imagine.

I mean a proxy as an object that 'looks' like a reference.
It's often necessary to for (proxied) collections of objects that can not be
accessed directly, via regular pointers/references.

> I read in some tutorial that using auto&& worked in all situations, including
> when the returned reference was const, so now I just use that everywhere.

auto&& is most useful when container<T>::reference returns a proxy object,
which would not bind to auto& (it would bind to auto const& though).

> In fact the nature of the object which is returned depends on the iterator,
> and as documented in the API, for the sake of implementation simplicity, those
> iterators do not necessarily respect the whole semantics of iterators.
>
> That is if you do
>
> auto iter = poLineString->begin();
> OGRPoint& oPoint = *iter;
> ++iter;
> OGRPoint& oAnotherPoint = *iter;
> In fact &oAnotherPoint == &oPoint.

At first, it seems like they model the input iterators, single-pass
iterator category
http://en.cppreference.com/w/cpp/concept/InputIterator

> So basically you should only use them in range based loops, where you don't
> really manipulate the iterator and cannot do stuff like the above.

Or, use like any input iterators, seems safe.

> I guess the "correct" solution would have been to return a OGRPointProxy that
> would keep a link to the linestring and the point index. But not sure how you
> would do that for a const iterator where the operator * is supposed to
> returned a const reference...

Or reference-like proxy like std::vector<bool> does.

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Use of C++ iterators in API

Kurt Schwehr-2
The auto&& is really confusing.

-kurt

On Tue, Apr 10, 2018 at 2:41 PM, Mateusz Loskot <[hidden email]> wrote:
On 10 April 2018 at 23:06, Even Rouault <[hidden email]> wrote:
> On mardi 10 avril 2018 22:34:56 CEST Mateusz Loskot wrote:
>> On 10 April 2018 at 22:06, Even Rouault <[hidden email]> wrote:
>> >
>> > Just wanted to advertize the recent new introduction of C++ iterators
>> > (...)
>>
>> Good one, thanks!
>>
>> >     for( auto&& poLayer: poDS->GetLayers() )
>>
>> Do the iterators return a proxy reference hence the tutorial prefers
>> auto&& over plain auto&?
>> (ie. like std::vector<bool> iterator does)
>
> Not completely sure to understand what a proxy reference is (C++11 stuff is
> still very new to me, and I don't pretend to master C++98, so ...), but I feel
> that must be close to what I have used in some cases, particularly wen
> iterating over fields of a features, or points of a linestring.

I didn't look at the iterators impl. yet.
I just looked at your example and wondered if there are proxies
- for GDAL/OGR, those could be necessary, I imagine.

I mean a proxy as an object that 'looks' like a reference.
It's often necessary to for (proxied) collections of objects that can not be
accessed directly, via regular pointers/references.

> I read in some tutorial that using auto&& worked in all situations, including
> when the returned reference was const, so now I just use that everywhere.

auto&& is most useful when container<T>::reference returns a proxy object,
which would not bind to auto& (it would bind to auto const& though).

> In fact the nature of the object which is returned depends on the iterator,
> and as documented in the API, for the sake of implementation simplicity, those
> iterators do not necessarily respect the whole semantics of iterators.
>
> That is if you do
>
> auto iter = poLineString->begin();
> OGRPoint& oPoint = *iter;
> ++iter;
> OGRPoint& oAnotherPoint = *iter;
> In fact &oAnotherPoint == &oPoint.

At first, it seems like they model the input iterators, single-pass
iterator category
http://en.cppreference.com/w/cpp/concept/InputIterator

> So basically you should only use them in range based loops, where you don't
> really manipulate the iterator and cannot do stuff like the above.

Or, use like any input iterators, seems safe.

> I guess the "correct" solution would have been to return a OGRPointProxy that
> would keep a link to the linestring and the point index. But not sure how you
> would do that for a const iterator where the operator * is supposed to
> returned a const reference...

Or reference-like proxy like std::vector<bool> does.

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev



--

_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Use of C++ iterators in API

Even Rouault-2
On mercredi 11 avril 2018 05:10:53 CEST Kurt Schwehr wrote:
> The auto&& is really confusing.

A experimentally working alternative with miminal use of '&' is

    for( auto poLayer: poDS->GetLayers() )  <-- this is a plain OGRLayer*
    {
        for( auto& poFeature: *poLayer )  <-- this is a unique_ptr<OGRFeature> so needs to be obtained by reference (otherwise compiler complains about use of deleted function unique_ptr(const unique_ptr&) = delete
        {
            for( auto& oField: *poFeature )  <-- this is a OGRFeature::FieldValue object that has no copy constructor (intended, we don't want user to be able to instanciate standalone FieldValue)

Does that look better ?

In all those above cases, the auto could also be const'ified, so the stricter typing would be

    for( const auto poLayer: poDS->GetLayers() )
    {
        for( const auto& poFeature: *poLayer )
        {
            for( const auto& oField: *poFeature )

Should we put that instead ?

--
Spatialys - Geospatial professional services
http://www.spatialys.com
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Use of C++ iterators in API

andrew.bell.ia@gmail.com

On Wed, Apr 11, 2018 at 8:40 AM, Even Rouault <[hidden email]> wrote:
On mercredi 11 avril 2018 05:10:53 CEST Kurt Schwehr wrote:
> The auto&& is really confusing.

A experimentally working alternative with miminal use of '&' is

    for( auto poLayer: poDS->GetLayers() )  <-- this is a plain OGRLayer*
    {
        for( auto& poFeature: *poLayer )  <-- this is a unique_ptr<OGRFeature> so needs to be obtained by reference (otherwise compiler complains about use of deleted function unique_ptr(const unique_ptr&) = delete
        {
            for( auto& oField: *poFeature )  <-- this is a OGRFeature::FieldValue object that has no copy constructor (intended, we don't want user to be able to instanciate standalone FieldValue)

Does that look better ?

In all those above cases, the auto could also be const'ified, so the stricter typing would be

    for( const auto poLayer: poDS->GetLayers() )
    {
        for( const auto& poFeature: *poLayer )
        {
            for( const auto& oField: *poFeature ) 

Should we put that instead ?

I believe the universal reference (&&) has no purpose in this context. It will decay to a standard reference.  So what you post is more clear.

--
Andrew Bell
[hidden email]

_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Use of C++ iterators in API

Mateusz Loskot
In reply to this post by Even Rouault-2
On 11 April 2018 at 14:40, Even Rouault <[hidden email]> wrote:
> On mercredi 11 avril 2018 05:10:53 CEST Kurt Schwehr wrote:
>> The auto&& is really confusing.
>
> A experimentally working alternative with miminal use of '&' is
>
>     for( auto poLayer: poDS->GetLayers() )  <-- this is a plain OGRLayer*

IMHO, it's better to get users familar with these two:

auto const& - as preferred default
auto& - where one requires, explicitly

It should cover 99.9% of uses :)

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Use of C++ iterators in API

Alex HighViz
In reply to this post by Even Rouault-2
> Even:
> In fact the nature of the object which is returned depends on the iterator, and as documented in the API, for the sake of implementation simplicity, those iterators do not necessarily respect the whole semantics of iterators.

I don't think that is necessary, at least in the case of Layers you can conform to the InputIterator concept without much extra work

[snip]

> So basically you should only use them in range based loops, where you don't really manipulate the iterator and cannot do stuff like the above.

That is also the limitation of InputIterator.

> Improvements (if doable and not adding (too much) overhead to the underlying objects) are welcome of course.

I would recommend that Layers::iterator is updated to conform to the C++ iterator concepts (to the very least to the InputIterator concept).
It still requires the following:
- five typedefs (value_type, reference, pointer, iterator_category, difference_type)
- three constructors (default, copy, move)
- two assignment operators (copy, move)
- post-increment operator (operator++(int))

See also http://en.cppreference.com/w/cpp/concept/InputIterator.

*************************************************
Regarding the for(auto&& layer : layers) discussion:


The return type of Layers::Iterator::operator*() is OGRLayer*.

Therefore, the following:

    for(auto& layer : layers)

is incorrect and should not compile, because it binds a temporary to a reference.
 
Furthermore, the following:
 
    for(const auto& layer: layers)

is only correct because an assignment of a temporary to a const reference extends its lifetime, and it is therefore equivalent to

   for(const auto layer : layers)

Considering the potential confusion, you can just avoid the issue completely and write:

   for(const OGRLayer* layer : layers)


Kind regards, Alex
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Use of C++ iterators in API

Even Rouault-2
> I would recommend that Layers::iterator is updated to conform to the C++
> iterator concepts (to the very least to the InputIterator concept).
 It
> still requires the following:
> - five typedefs (value_type, reference, pointer, iterator_category,
> difference_type)
 - three constructors (default, copy, move)
> - two assignment operators (copy, move)
> - post-increment operator (operator++(int))
>
> See also http://en.cppreference.com/w/cpp/concept/InputIterator.

I just planted the seed. Contributions (preferably as pull requests)
welcome...

> Considering the potential confusion, you can just avoid the issue completely
> and write:
 
>    for(const OGRLayer* layer : layers)

Fair enough. I'll use this.


--
Spatialys - Geospatial professional services
http://www.spatialys.com
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Use of C++ iterators in API

Alex HighViz

From: Even Rouault [mailto:[hidden email]]


> I just planted the seed. Contributions (preferably as pull requests) welcome...

Ok just did that PR #411.


_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev