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

[Even Rouault]

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