[Proj] C++ coding practices w.r.t object ownership

[Even Rouault]

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