Reviewing std::auto_ptr spaghetti

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Reviewing std::auto_ptr spaghetti

Mateusz Loskot
Hi,

In C++, well-established patterns when/how to use std::auto_ptr are:

void sink(std::auto_ptr<T> p);
std::auto_ptr<T> source();

TL;TR: Is it safe to assume GEOS follows the patterns?


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?

For example, in LineSegmentVisitor and possibly (lot) more places.

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
_______________________________________________
geos-devel mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/geos-devel
Reply | Threaded
Open this post in threaded view
|

Re: Reviewing std::auto_ptr spaghetti

Sandro Santilli-4
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. Very dangerous for C++ interface, but safe for C-API.

Best we can do is document when needed.

--strk;
_______________________________________________
geos-devel mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/geos-devel
Reply | Threaded
Open this post in threaded view
|

Re: Reviewing std::auto_ptr spaghetti

Mateusz Loskot
On 6 September 2017 at 17:17, Sandro Santilli <[hidden email]> 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
_______________________________________________
geos-devel mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/geos-devel