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

[Kurt Schwehr]

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 <even.rouault at spatialys.com>
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
> Proj at lists.maptools.org
> http://lists.maptools.org/mailman/listinfo/proj
>



-- 
--
http://schwehr.org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.maptools.org/pipermail/proj/attachments/20180527/24433cbe/attachment-0001.htm