[geos-devel] Reviewing std::auto_ptr spaghetti

[Mateusz Loskot]

On 6 September 2017 at 17:17, Sandro Santilli <strk at kbt.io> wrote:
> On Wed, Sep 06, 2017 at 04:24:45PM +0200, Mateusz Loskot wrote:
>
>> In the code, there are uses of std::auto_ptr that are hard to reason about
>> what makes me suspicious:
>>
>> class T
>> {
>>   auto_ptr items;
>> public
>>     auto_ptr<A> getItems()
>>     {
>>         return items;
>>     }
>> };
>>
>> Is the intention to give up the object's internal resource, really?
>
> Yes. I agree it's a weird pattern, but it's what got us to the current
> leak-free state.

Yes, that was better than nothing.

> Very dangerous for C++ interface, but safe for C-API.

I'm not sure I understand the difference, unless you assume
a) 100% LOC coverage with tests and
b) GEOS never misuses such classes/interfaces as pointed above

> Best we can do is document when needed.

Yes, but since it seems an enormous code review task,
I'm hardly optimistic it's feasible.

Practically possible migration away from auto_ptr is:
1. s/auto_ptr/unique_ptr/
2. Add std::move(ptr) wherever required.
3. Ensure GEOS compiles.
4. Ensure GEOS tests pass.
5. Document C++ API breakage.
6. Assume GEOS remains unbroken.

If I hear no objections, I will (try to) proceed as outlined.

Best regards,
-- 
Mateusz Loskot, http://mateusz.loskot.net