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

[Mateusz Loskot]

On 29 May 2018 at 12:10, Even Rouault <even.rouault at spatialys.com> 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