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

[Even Rouault]

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