C++ coding practices w.r.t object ownership

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

C++ coding practices w.r.t object ownership

Even Rouault-2
Hi,

I just wanted to give a very early preview of my prototyping of the ISO
191111:2018 class hiearchy, so we can discuss some coding practices (so
non-"cosmetic" aspects of coding), especially about fundamental conventions
deeply impacting the API, particularly regarding object ownership.

This is not material ready for detailed review: there are lots of TODOs, no
docs, mostly untested, doesn't do anything really useful yet, etc.

I've uploaded the Doxygen doc generated from the work in progress. Two useful
links to have an overview:

Diagram of class hiearchy:
http://even.rouault.free.fr/proj_cpp_api/html/inherits.html

Namespaces/packages and classes
http://even.rouault.free.fr/proj_cpp_api/html/annotated.html

Example of the CRS package (formatted with clang-format):
https://github.com/rouault/proj.4/blob/iso19111/src/crs.hh
https://github.com/rouault/proj.4/blob/iso19111/src/crs.cpp

Test program:
https://github.com/rouault/proj.4/blob/iso19111/test/test_cpp.cpp

A few principles I've adopted:
- done in a way such that memory leaks cannot happen (internal use of
unique_ptr)
- all private members are strictly hidden in the header file, by delegating to
a internal private struct in the .cpp file. This way we can add private
members without impacting the ABI
- from the GeoAPI design, I've kept the apparent design constraint of having
immutable objects once constructed.
- the getters generally return a pointer or const reference to the internal
structure members. Which means that those objects must not be used after their
belonging object has been destroyed

~~~~~~

There are different ways that could be used for object life-cycle management:

1) the one I took in this first sketch: we pass objects by const reference,
and methods/constructors that need to keep them, create a copy.

For example:

GeographicCRS createGeographicCRS(const PropertyMap& properties,
                                    const GeodeticReferenceFrame& datum,
                                    const EllipsoidalCS& cs);

will construct a GeographicCRS object that will duplicate datum and cs
internally.

2) another one we could imagine: use shared pointers

The above would become:

std::shared_ptr<GeographicCRS>
 createGeographicCRS(const PropertyMap& properties,
                      std::shared_ptr<GeodeticReferenceFrame> datum,
                      std::shared_ptr<EllipsoidalCS> cs);

3) have ad-hoc conventions regarding pointer ownership. But we probably want
to avoid this as this is error-prone for API users (incluing PROJ internal
use)

~~~~~~

Comparing the 2 first approaches:

Copying approach:
- pros:  no null pointer checking needed when acception a const& argument
- cons:
  * can involve a lot of copying around when building complex object
hiearchies
  * internally involves a kind of hack ( clone() method ) so that the
duplication correctly clones instances of derived classes. To be clear: let's
consider the situation where you have a constructor that accepts a Base& or
Base* argument, that you need to store as a class member. If you call it with
a Derived object, you want a instance of Derived to be cloned, not its Base-
only part.

shared_ptr approach:
- pros:
   * reduced copying, avoid the cloning hack
   * we could return shared_ptr objects in getters, making them super safe to
use (the return can be used after the belonging object has been destroyed).
- cons:
 * null-pointer situations can happen and must be checked / documented

My feeling is that the shared_ptr approach could be better, although CRS
instanciation is probably not critical in most use cases. Of course shared_ptr
aren't completely free performance-wise, but I feel this would be more
efficient than deep copying of complex objects.

And, to save explicit null checking in situations where we don't want null
pointers, we could wrap the shared_ptr in a nn<> class [1] that guarantees
that the hold pointer is not null.

We would then have:
GeographicCRS_nnshptr
   createGeographicCRS(const PropertyMap& properties,
                          GeodeticReferenceFrame_nnshptr datum,
                       EllipsoidalCS_nnshptr cs);

with using xxxx_nnshptr = nn_shared_ptr<xxxx>;

Thoughts ?

Even

[1]  https://github.com/dropbox/nn/blob/master/nn.hpp

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

Re: C++ coding practices w.r.t object ownership

Martin Desruisseaux-3
Hello Even

Thanks for this first draft, it looks promising!

One quick comment: it may be worth to move CI_Citation, EX_Extent and
MD_Identifier, currently in "common", to their own "metadata" package.
The reason is that even if they are currently the main ISO 19115
structures of interest for Proj, in the future GDAL may be interested in
more ISO 19115 metadata structures (e.g. GeographicBoundingBox,
SampleDimension, CoverageDescription, etc.).

    Martin


_______________________________________________
Proj mailing list
[hidden email]
http://lists.maptools.org/mailman/listinfo/proj
Reply | Threaded
Open this post in threaded view
|

Re: C++ coding practices w.r.t object ownership

Kurt Schwehr-2
In reply to this post by Even Rouault-2
I maybe prefer 1, but I've not really used shared_ptr myself.  I suggest keeping the static Builder concept in mid for object creation.  It gives a lot of control for allowing creation of const instances.

You may want to consider some singletons.

Some thoughts on the code since it's out there...


SingleCRS has PROJ_OPAQUE_PRIVATE_DATA, so why do the derived classes like GeodeticCRS also have PROJ_OPAQUE_PRIVATE_DATA?


Two layout requests:

1. Can you switch to public, protected, private order in classes.  That usually more closely matches what a reader is likely needing to know.

2. Please try to avoid usings namespace polluting usings like this:

using namespace common;
using namespace datum;
using namespace cs;
using namespace operation;
using namespace util;

I find it easier to follow when I see:

using ::osgeo::proj::datum::Datum;  // etc.

That way you don't have to worry about name collisions or other surprises.
And it provides a concise list to the reader.

Or just use the namespace when you use the particular... e.g.

const auto cs_helper = std::make_unique<cs::Helper>(foo, bar);


No need for virtual and override.  virtual is redundant

virtual const GeodeticReferenceFrame *datum() const override;

Should just be:

const GeodeticReferenceFrame *datum() const override;


With these 2 methods, why is one returning a const ptr and the other a const ref?

virtual const GeodeticReferenceFrame *datum() const override;
const GeodeticCS &geodeticCoordinateSystem() const;



On Sun, May 27, 2018 at 8:35 AM, Even Rouault <[hidden email]> wrote:
Hi,

I just wanted to give a very early preview of my prototyping of the ISO
191111:2018 class hiearchy, so we can discuss some coding practices (so
non-"cosmetic" aspects of coding), especially about fundamental conventions
deeply impacting the API, particularly regarding object ownership.

This is not material ready for detailed review: there are lots of TODOs, no
docs, mostly untested, doesn't do anything really useful yet, etc.

I've uploaded the Doxygen doc generated from the work in progress. Two useful
links to have an overview:

Diagram of class hiearchy:
http://even.rouault.free.fr/proj_cpp_api/html/inherits.html

Namespaces/packages and classes
http://even.rouault.free.fr/proj_cpp_api/html/annotated.html

Example of the CRS package (formatted with clang-format):
https://github.com/rouault/proj.4/blob/iso19111/src/crs.hh
https://github.com/rouault/proj.4/blob/iso19111/src/crs.cpp

Test program:
https://github.com/rouault/proj.4/blob/iso19111/test/test_cpp.cpp

A few principles I've adopted:
- done in a way such that memory leaks cannot happen (internal use of
unique_ptr)
- all private members are strictly hidden in the header file, by delegating to
a internal private struct in the .cpp file. This way we can add private
members without impacting the ABI
- from the GeoAPI design, I've kept the apparent design constraint of having
immutable objects once constructed.
- the getters generally return a pointer or const reference to the internal
structure members. Which means that those objects must not be used after their
belonging object has been destroyed

~~~~~~

There are different ways that could be used for object life-cycle management:

1) the one I took in this first sketch: we pass objects by const reference,
and methods/constructors that need to keep them, create a copy.

For example:

GeographicCRS createGeographicCRS(const PropertyMap& properties,
                                    const GeodeticReferenceFrame& datum,
                                    const EllipsoidalCS& cs);

will construct a GeographicCRS object that will duplicate datum and cs
internally.

2) another one we could imagine: use shared pointers

The above would become:

std::shared_ptr<GeographicCRS>
 createGeographicCRS(const PropertyMap& properties,
                      std::shared_ptr<GeodeticReferenceFrame> datum,
                      std::shared_ptr<EllipsoidalCS> cs);

3) have ad-hoc conventions regarding pointer ownership. But we probably want
to avoid this as this is error-prone for API users (incluing PROJ internal
use)

~~~~~~

Comparing the 2 first approaches:

Copying approach:
- pros:  no null pointer checking needed when acception a const& argument
- cons:
  * can involve a lot of copying around when building complex object
hiearchies
  * internally involves a kind of hack ( clone() method ) so that the
duplication correctly clones instances of derived classes. To be clear: let's
consider the situation where you have a constructor that accepts a Base& or
Base* argument, that you need to store as a class member. If you call it with
a Derived object, you want a instance of Derived to be cloned, not its Base-
only part.

shared_ptr approach:
- pros:
   * reduced copying, avoid the cloning hack
   * we could return shared_ptr objects in getters, making them super safe to
use (the return can be used after the belonging object has been destroyed).
- cons:
 * null-pointer situations can happen and must be checked / documented

My feeling is that the shared_ptr approach could be better, although CRS
instanciation is probably not critical in most use cases. Of course shared_ptr
aren't completely free performance-wise, but I feel this would be more
efficient than deep copying of complex objects.

And, to save explicit null checking in situations where we don't want null
pointers, we could wrap the shared_ptr in a nn<> class [1] that guarantees
that the hold pointer is not null.

We would then have:
GeographicCRS_nnshptr
   createGeographicCRS(const PropertyMap& properties,
                                         GeodeticReferenceFrame_nnshptr datum,
                                              EllipsoidalCS_nnshptr cs);

with using xxxx_nnshptr = nn_shared_ptr<xxxx>;

Thoughts ?

Even

[1]  https://github.com/dropbox/nn/blob/master/nn.hpp

--
Spatialys - Geospatial professional services
http://www.spatialys.com
_______________________________________________
Proj mailing list
[hidden email]
http://lists.maptools.org/mailman/listinfo/proj



--

_______________________________________________
Proj mailing list
[hidden email]
http://lists.maptools.org/mailman/listinfo/proj
Reply | Threaded
Open this post in threaded view
|

Re: C++ coding practices w.r.t object ownership

Even Rouault-2
In reply to this post by Martin Desruisseaux-3
On dimanche 27 mai 2018 18:05:14 CEST Martin Desruisseaux wrote:
> Hello Even
>
> Thanks for this first draft, it looks promising!
>
> One quick comment: it may be worth to move CI_Citation, EX_Extent and
> MD_Identifier, currently in "common", to their own "metadata" package.

OK, done

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

Re: C++ coding practices w.r.t object ownership

Even Rouault-2
In reply to this post by Kurt Schwehr-2
On dimanche 27 mai 2018 09:14:16 CEST Kurt Schwehr wrote:
> I maybe prefer 1, but I've not really used shared_ptr myself.  

I see the libkml API is close to the shared_ptr approach (except it uses a
boost::intrusive_ptr which is a variation of shared_ptr), where you pass and
get those pointer wrappers all the time throughout the API.

kml/dom/kml_ptr.h:typedef boost::intrusive_ptr<Element> ElementPtr;

I'm a bit concerned by all the hidden cost of object duplication with the
approach I tried. And if you instanciate various projected coordinate systems
based on the same datum, currently all the datum information would be
duplicated in each PCS object, whereas with a pointer based approach you would
have just one instance of it.

> I suggest
> keeping the static Builder concept in mid for object creation.  It gives a
> lot of control for allowing creation of const instances.

Do you have links for best practice regarding this ?

>
> You may want to consider some singletons.

Yes, more work needed on object instanciation.

> SingleCRS has PROJ_OPAQUE_PRIVATE_DATA, so why do the derived classes
> like GeodeticCRS
> also have PROJ_OPAQUE_PRIVATE_DATA?

Because they can have their own additional private members.

> 1. Can you switch to public, protected, private order in classes.

Done

>
> 2. Please try to avoid usings namespace polluting usings like this:

Done.

>
> No need for virtual and override.  virtual is redundant

Done

>
> With these 2 methods, why is one returning a const ptr and the other a
> const ref?
>
> virtual const GeodeticReferenceFrame *datum() const override;
> const GeodeticCS &geodeticCoordinateSystem() const;
>

datum might be optional (there is either a datum or a datumEnsemble in a
SingleCRS), hence a pointer, whereas the coordinate system is mandatory

with the shared pointer approach, we could return pointers in both cases:
const GeodeticReferenceFrame_shptr datum() const
const GeodeticCS_nnshptr geodeticCoordinateSystem() const

Even

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

Re: C++ coding practices w.r.t object ownership

Martin Desruisseaux-3
In reply to this post by Even Rouault-2

I have one minor suggestion. In crs.hh file, there is the following comment in the GeodeticCRS class:

// slight departure from standard. We can't override coordinateSystem() since
// GeographicCRS::coordinateSystem() returns cs::EllipsoidalCS which is not
// derived from GeodeticCS (GeodeticCS is a pseudo union) ...
const cs::GeodeticCS &geodeticCoordinateSystem() const;

I would suggest to not try too hard to represent verbatim the « union » constructs from ISO 19111, but rather focus on the intent. With this GeodeticCS « union », ISO 19111 is trying to said that a GeodeticCRS shall be associated only to an EllipsoidCS, CartesianCS or SphericalCS. If it is possible to express this constraint in the C/C++ language (with C union or whatever other construct may fit), good! If it is not practical, then I think it is okay to just state the constraint in the documentation. This is what we have done in Java for instance. Having a second property returning a "CS-like" object gives the impression that SingleCRS can be associated to two coordinate systems, which may be confusing.

Note: ISO 19111 do not define GeodeticCS as a parent of EllipsoidCS, CartesianCS and SphericalCS because conceptually, "GeodeticCS" is not really a kind of coordinate system. Another reason is that such hierarchy would be very specific to GeodeticCRS needs and may fit the needs of other CRS.

    Martin



_______________________________________________
Proj mailing list
[hidden email]
http://lists.maptools.org/mailman/listinfo/proj
Reply | Threaded
Open this post in threaded view
|

Re: C++ coding practices w.r.t object ownership

Even Rouault-2
On dimanche 27 mai 2018 22:14:50 CEST Martin Desruisseaux wrote:

> I have one minor suggestion. In crs.hh file, there is the following
> comment in the GeodeticCRS class:
>
>     // slight departure from standard. We can't override coordinateSystem()
> since // GeographicCRS::coordinateSystem() returns cs::EllipsoidalCS which
> is not // derived from GeodeticCS (GeodeticCS is a pseudo union) ...
>     const cs::GeodeticCS &geodeticCoordinateSystem() const;
>
> I would suggest to not try too hard to represent verbatim the « union »
> constructs from ISO 19111, but rather focus on the intent.

Agreed. I wasn't really satisfied with this. A documentation note will do.

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

Re: C++ coding practices w.r.t object ownership

Even Rouault-2
In reply to this post by Even Rouault-2
> I'm a bit concerned by all the hidden cost of object duplication with the
> approach I tried. And if you instanciate various projected coordinate
> systems based on the same datum, currently all the datum information would
> be duplicated in each PCS object, whereas with a pointer based approach you
> would have just one instance of it.
>

I've experienced with the shared_ptr approach in
https://github.com/rouault/proj.4/commit/85f619cbae7a0b2071681e150f93a5301fa32098

The most satisfactory result is the removal of the clone() hacks. Side effect is that the creation of
the GeographicCRS is 30% faster than the previous approach (measured on a 1 million iterations loop)

Even

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

Re: C++ coding practices w.r.t object ownership

Mateusz Loskot
On Sun, 27 May 2018, 23:26 Even Rouault, <[hidden email]> wrote:
> I'm a bit concerned by all the hidden cost of object duplication with the
> approach I tried. And if you instanciate various projected coordinate
> systems based on the same datum, currently all the datum information would
> be duplicated in each PCS object, whereas with a pointer based approach you
> would have just one instance of it.
>

I've experienced with the shared_ptr approach in
https://github.com/rouault/proj.4/commit/85f619cbae7a0b2071681e150f93a5301fa32098

std::make_shared is the canonical 
and efficient way to manage shared_ptr instances.
The new should be hardly ever needed.

BTW, I've been reading the thread, nothing major to add though - I myself agree with Kurt's points. 
For minor points, little point to discuss those and generate unfocused traffic. 


Mateusz Loskot, [hidden email]
(Sent from mobile)


_______________________________________________
Proj mailing list
[hidden email]
http://lists.maptools.org/mailman/listinfo/proj
Reply | Threaded
Open this post in threaded view
|

Re: C++ coding practices w.r.t object ownership

Kurt Schwehr-2
Agreed... Anytime you write "new" you probably shouldn't unless you are working with a legacy API.

Trying to write c++ with a phone sucks...

On Sun, May 27, 2018, 2:51 PM Mateusz Loskot <[hidden email]> wrote:
On Sun, 27 May 2018, 23:26 Even Rouault, <[hidden email]> wrote:
> I'm a bit concerned by all the hidden cost of object duplication with the
> approach I tried. And if you instanciate various projected coordinate
> systems based on the same datum, currently all the datum information would
> be duplicated in each PCS object, whereas with a pointer based approach you
> would have just one instance of it.
>

I've experienced with the shared_ptr approach in
https://github.com/rouault/proj.4/commit/85f619cbae7a0b2071681e150f93a5301fa32098

std::make_shared is the canonical 
and efficient way to manage shared_ptr instances.
The new should be hardly ever needed.

BTW, I've been reading the thread, nothing major to add though - I myself agree with Kurt's points. 
For minor points, little point to discuss those and generate unfocused traffic. 


Mateusz Loskot, [hidden email]
(Sent from mobile)

_______________________________________________
Proj mailing list
[hidden email]
http://lists.maptools.org/mailman/listinfo/proj

_______________________________________________
Proj mailing list
[hidden email]
http://lists.maptools.org/mailman/listinfo/proj
Reply | Threaded
Open this post in threaded view
|

Re: C++ coding practices w.r.t object ownership

J Luis

 

 

From: [hidden email] <[hidden email]> On Behalf Of Kurt Schwehr
Sent: Sunday, May 27, 2018 11:26 PM
To: PROJ.4 and general Projections Discussions <[hidden email]>
Subject: Re: [Proj] C++ coding practices w.r.t object ownership

 

 

Trying to write c++ with a phone sucks...

 

 

Sure, it’s a C phone.

 


_______________________________________________
Proj mailing list
[hidden email]
http://lists.maptools.org/mailman/listinfo/proj
Reply | Threaded
Open this post in threaded view
|

Re: C++ coding practices w.r.t object ownership

Martin Desruisseaux-3
In reply to this post by Even Rouault-2
Le 27/05/2018 à 20:11, Even Rouault a écrit :
>> One quick comment: it may be worth to move CI_Citation, EX_Extent and
>> MD_Identifier, currently in "common", to their own "metadata" package.
> OK, done

I think that the "CI_", "EX_" and "MD_" prefixes can be omitted. They
duplicate namespaces/packages in C++/Java and recent ISO/TC211 policy
(in my understanding) is to drop them in newer standards. For example
"Datum" in ISO 19111:2018 was "CD_Datum" in ISO 19111:2007.

PositionalAccuracy (currently in "common" namespace) belong to
"metadata" namespace too.

Regarding the object ownership discussion, one more criterion that may
be worth to consider is whether the proposed alternatives support cyclic
references. For example ProjectedCRS.conversion references a Conversion
object, which could in turn reference back the ProjectedCRS in its
Conversion.targetCRS attribute. ISO 19111 allows to break this
circularity by allowing Conversion.source/targetCRS to be null in such
cases. But I find convenient to nevertheless provide the reference value
when the language/framework support cyclic references since it makes
easier to use a Conversion instance without carrying extra information
about its context. Circularity may also happen in ISO 19115 metadata,
e.g. between Platform and Instrument classes.

    Martin


_______________________________________________
Proj mailing list
[hidden email]
http://lists.maptools.org/mailman/listinfo/proj
Reply | Threaded
Open this post in threaded view
|

Re: C++ coding practices w.r.t object ownership

Martin Desruisseaux-3
Le 29/05/2018 à 09:31, Martin Desruisseaux a écrit :

> PositionalAccuracy (currently in "common" namespace) belong to
> "metadata" namespace too.
>
Oups! I forgot that it moved in another standard, ISO 19157. So it would
rather be a new "quality" namespace. But quality information could also
be considered as a kind of metadata; I'm neutral on that.

    Martin


_______________________________________________
Proj mailing list
[hidden email]
http://lists.maptools.org/mailman/listinfo/proj
Reply | Threaded
Open this post in threaded view
|

Re: C++ coding practices w.r.t object ownership

Even Rouault-2
In reply to this post by Martin Desruisseaux-3
> I think that the "CI_", "EX_" and "MD_" prefixes can be omitted. They
> duplicate namespaces/packages in C++/Java and recent ISO/TC211 policy
> (in my understanding) is to drop them in newer standards. For example
> "Datum" in ISO 19111:2018 was "CD_Datum" in ISO 19111:2007.

Yes, sounds reasonable

>
> PositionalAccuracy (currently in "common" namespace) belong to
> "metadata" namespace too.

OK

>
> Regarding the object ownership discussion, one more criterion that may
> be worth to consider is whether the proposed alternatives support cyclic
> references. For example ProjectedCRS.conversion references a Conversion
> object, which could in turn reference back the ProjectedCRS in its
> Conversion.targetCRS attribute. ISO 19111 allows to break this
> circularity by allowing Conversion.source/targetCRS to be null in such
> cases. But I find convenient to nevertheless provide the reference value
> when the language/framework support cyclic references since it makes
> easier to use a Conversion instance without carrying extra information
> about its context.
Yes, that's an annoying point I noticed yesterday when looking more closely at
this, since cyclic references are unfriendly with shared pointers.

The solution is that CoordinateOperation stores sourceCRS and targetCRS as
std::weak_ptr internally. In the sourceCRS() and targetCRS() getters, those
weak pointers are converted to shared pointers with .lock() (the
shared_pointer being then potentially null if the owning ProjectedCRS has been
destroyed in between). So users of the API only see shared pointers. And add  
a documentation note of sourceCRS() and targetCRS() about the particular case
of ProjectedCRS.derivingConversion.

Bonus point: when the CoordinateOperation is created in all other contexts
than the derivingConversion of a ProjectedCRS, then CoordinateOperation can
also have internal shared_ptr so as to make sure than the source and target
CRS are kept alive, and the weak_ptr conversion to a shared_one always return
a non null pointer.

Attached a POC (simplified but representative of the above situation) that
works pretty well: no memory leak, and safe pointer usage.


> Circularity may also happen in ISO 19115 metadata,
> e.g. between Platform and Instrument classes.

OK. I don't think I'll need those ones. Enough classes for now !

Even

--
Spatialys - Geospatial professional services
http://www.spatialys.com
_______________________________________________
Proj mailing list
[hidden email]
http://lists.maptools.org/mailman/listinfo/proj

testweak.cpp (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: C++ coding practices w.r.t object ownership

Mateusz Loskot
On 29 May 2018 at 12:10, Even Rouault <[hidden email]> wrote:

>>
>> Regarding the object ownership discussion, one more criterion that may
>> be worth to consider is whether the proposed alternatives support cyclic
>> references. For example ProjectedCRS.conversion references a Conversion
>> object, which could in turn reference back the ProjectedCRS in its
>> Conversion.targetCRS attribute. ISO 19111 allows to break this
>> circularity by allowing Conversion.source/targetCRS to be null in such
>> cases. But I find convenient to nevertheless provide the reference value
>> when the language/framework support cyclic references since it makes
>> easier to use a Conversion instance without carrying extra information
>> about its context.
>
> Yes, that's an annoying point I noticed yesterday when looking more closely at
> this, since cyclic references are unfriendly with shared pointers.
>
> The solution is that CoordinateOperation stores sourceCRS and targetCRS as
> std::weak_ptr internally. In the sourceCRS() and targetCRS() getters, those
> weak pointers are converted to shared pointers with .lock() (the
> shared_pointer being then potentially null if the owning ProjectedCRS has been
> destroyed in between). So users of the API only see shared pointers. And add
> a documentation note of sourceCRS() and targetCRS() about the particular case
> of ProjectedCRS.derivingConversion.

The dual mode of the Conversion class obscurse the interface a bit.
Since the Conversion::sourceCRS result can be empty
it now has basically a raw pointer semantic veiled behind shared_ptr.

IOW, a user writes function taking ConversionPtr, she must take the
dual-mode into account - but will she, if she sees the function
returns shared_ptr?

void foo(ConversionPtr c)
{
    auto src = c->sourceCRS();
    if (!src) throw std::invalid_argument("empty source CRS");
    //...
}

The Conversion class kind of breaks the Liskov substitution principle too,
as it defines two kinds of types (behaviours).

On note on the class inteface side:
I'd suggest to avoid boolean types for arguments controlling ownership
behaviour,
especially booleans with default values,
and especially for controlling crucial behaviours.

I mean, whenever I see prototypes like this, it's a code smell to my senses:

static ConversionPtr create(CRSPtr crs, bool
creatorStoresConversionInternally = false)


It makes a bit bad interface and client code error prone,
raises questions, makes code harder to read and reason about.
There are two modes, but three invocation conventions:

Conversion::Create(p)
Conversion::Create(p, true)
Conversion::Create(p, false)

The C++ library could bundle shared_ptr and weak_ptr into one type with extra
constructor arguments, but they didn't for reason.

For crucial behaviours, an explicit interface and usage intention is better, eg.

enum class Ownership { None, Steal };
static ConversionPtr create(CRSPtr crs, Ownership own);

I sense there may be more classes where such dual ownership mode will be
required to avoid cycles, then the sane enum and pattern could be used
and user can quickly learn what it means when she sees it in the code.

> Attached a POC (simplified but representative of the above situation) that
> works pretty well: no memory leak, and safe pointer usage.

It does, because std::shared_ptr is a garbage collector :-)
the Java way to deal with unspecified lifetime of objects.


Has it been considered to use some sort of in-memory registry (a singleton)
to  manage CRS objects with its users (ProjectedCRS, Conversion)
defined as observers?
The whole CRS hierarchy feels like objects for plain state only. So,
it feels like
there is a pool with shared immutable data could be used (or Flyweight pattern).

Another design improvement that may help to avoid cycles is to prefer passing
any objects with extrinsic states to methods directly, instead of storing as
members of client class/object.

Has it been simulated what would be incurret cost of not using shared_ptr
and using just copyable/moveable objects, or a smart pointer with value
semantic (with some uses of value_ptr if necessary).

I am certainly just looking at scrapes and not I haven't considered complete
design (I haven't even skimmed the ISO 19111), so I may be missing lots.
I just have an impression that shared_ptr-based garbage collection has
been adopted too eagerly.
It's so awesome, that once it's in, it's impossible to get rid of it :-)

"The obvious answer is not to use shared_ptr to objects which themselves might
contain a shared_ptr. shared_ptr is somewhat special, and should be
used sparingly."
- James Kanze (https://stackoverflow.com/a/9393099/151641)

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

Re: C++ coding practices w.r.t object ownership

Even Rouault-2
Mateusz,

> The dual mode of the Conversion class obscurse the interface a bit.
> Since the Conversion::sourceCRS result can be empty
> it now has basically a raw pointer semantic veiled behind shared_ptr.
>
> IOW, a user writes function taking ConversionPtr, she must take the
> dual-mode into account - but will she, if she sees the function
> returns shared_ptr?
>
> void foo(ConversionPtr c)
> {
>     auto src = c->sourceCRS();
>     if (!src) throw std::invalid_argument("empty source CRS");
>     //...
> }
>
> The Conversion class kind of breaks the Liskov substitution principle too,
> as it defines two kinds of types (behaviours).

That's annoying, but I can't see how to do better. This comes from the
modelization itself. A shared_ptr isn't assumed to hold always a non null
pointer.

> I mean, whenever I see prototypes like this, it's a code smell to my senses:
>
> static ConversionPtr create(CRSPtr crs, bool
> creatorStoresConversionInternally = false)

That was  just for the sake of a quick POC. Likely not the final interface.

> For crucial behaviours, an explicit interface and usage intention is better,
> eg.
>
> enum class Ownership { None, Steal };
> static ConversionPtr create(CRSPtr crs, Ownership own);
>
> I sense there may be more classes where such dual ownership mode will be
> required to avoid cycles,

Hopefully not.

> then the sane enum and pattern could be used
> and user can quickly learn what it means when she sees it in the code.

Users of the API should not have to deal with this. Objection construction
will be hidden by appropriate helpers.

>
> Has it been considered to use some sort of in-memory registry (a singleton)
> to  manage CRS objects with its users (ProjectedCRS, Conversion)
> defined as observers?
> The whole CRS hierarchy feels like objects for plain state only. So,
> it feels like
> there is a pool with shared immutable data could be used (or Flyweight
> pattern).
>

A pool could be used, but I don't think that's needed at the PROJ level for
now.

> Has it been simulated what would be incurret cost of not using shared_ptr
> and using just copyable/moveable objects

Compared to the initial approach ofusing copyable objects, there is a slight
performance improvement (see http://lists.maptools.org/pipermail/proj/2018-May/008258.html), but that's not my main motivation.

> , or a smart pointer with value
> semantic (with some uses of value_ptr if necessary).

Not sure to know what you are refering too here.

>
> I am certainly just looking at scrapes and not I haven't considered complete
> design (I haven't even skimmed the ISO 19111), so I may be missing lots. I
> just have an impression that shared_ptr-based garbage collection has been
> adopted too eagerly.
> It's so awesome, that once it's in, it's impossible to get rid of it :-)
>

I'll stick with shared pointers though, as they are working, and we can't have
pointers to rogue memory and all other pitfalls. Any other solution will have
also its advantages & drawbacks anyway (one feedback in another thread was
that GDAL C++ was confusing regarding object ownership)
Another main reason is that the UML modelling from which this is derived from
has probably be done with Java in mind as an implementation target. So shared
pointers are the closest tool we have to emulate Java garbage collection.

Even

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

Re: C++ coding practices w.r.t object ownership

Martin Desruisseaux-3

Le 29/05/2018 à 14:28, Even Rouault a écrit :

Another main reason is that the UML modelling from which this is derived from has probably be done with Java in mind as an implementation target. So shared pointers are the closest tool we have to emulate Java garbage collection.

Actually the design is a little bit more abstract. Normally (unless we did a mistake in the model), there is no requirement to use a garbage collector or not; the choice is up to implementer. ISO 19111 allows implementations without garbage collector by allowing some pointers to be null, for breaking cycles. For example, on a conceptual point of view Conversion.source/targetCRS should never be null. But in practice ISO 19111 nevertheless declares those associations as optional for implementers who want to avoid cycles between Conversion and ProjectedCRS.

So the choice for PROJ may be:

  • Avoid cycles as allowed by ISO 19111, at the cost of some inconveniences (e.g. usages of Conversion objects may require more contextual information).
  • More convenient Conversion objects, at the cost of cyclic references.

ProjectedCRS ⇋ Conversion is the main potential cycle I'm aware of. There may be another one in the new classes related to point motions or vertical transformations, but I'm not sure (I'm not yet familiar enough with that new part).

    Martin



_______________________________________________
Proj mailing list
[hidden email]
http://lists.maptools.org/mailman/listinfo/proj
Reply | Threaded
Open this post in threaded view
|

Re: C++ coding practices w.r.t object ownership

Mateusz Loskot
On 29 May 2018 at 15:01, Martin Desruisseaux
<[hidden email]> wrote:

> Le 29/05/2018 à 14:28, Even Rouault a écrit :
>>
>> Another main reason is that the UML modelling from which this is derived
>> from has probably be done with Java in mind as an implementation target. So
>> shared pointers are the closest tool we have to emulate Java garbage
>> collection.
>
> Actually the design is a little bit more abstract. Normally (unless we did a
> mistake in the model), there is no requirement to use a garbage collector or
> not; the choice is up to implementer. ISO 19111 allows implementations
> without garbage collector by allowing some pointers to be null, for breaking
> cycles. For example, on a conceptual point of view
> Conversion.source/targetCRS should never be null. But in practice ISO 19111
> nevertheless declares those associations as optional for implementers who
> want to avoid cycles between Conversion and ProjectedCRS.

In such case, final version of the Even's POC should/could state
the implementation requirement more explicitly, eg.

CRSPtr sourceCRS() const
{
    if (auto p = crsWeak_.lock())
        return p;
    else
        throw std::logic_error("dereferencing expired pointer");
}

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
_______________________________________________
Proj mailing list
[hidden email]
http://lists.maptools.org/mailman/listinfo/proj