GeometryPtr or ...

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

GeometryPtr or ...

Paul Ramsey
std::unique_ptr<Geometry> ?

We've got a mishmash in the code base, what should it be?
As a learner arriving at the code base, std::unique_ptr<Geometry> would have been easier, since then the semantics of the thing are more immediately transparent then. After working with it for a while, that's less of an issue because I've internalized the fact that GeometryPtr is a std::unique_ptr, but still. 

The best code styleguide is a consistent code base, so deciding and then globally changing makes the most sense, IMO.

P.

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

Re: GeometryPtr or ...

Daniel Baston
I'd agree; I find that typedefs like GeometryPtr generally obfuscate things. Although one can guess, it's not immediately obvious if GeometryPtr means Geometry*, unique_ptr<Geometry>, shared_ptr<Geometry>, or something else.

Dan

On Thu, Dec 6, 2018 at 5:46 PM Paul Ramsey <[hidden email]> wrote:
std::unique_ptr<Geometry> ?

We've got a mishmash in the code base, what should it be?
As a learner arriving at the code base, std::unique_ptr<Geometry> would have been easier, since then the semantics of the thing are more immediately transparent then. After working with it for a while, that's less of an issue because I've internalized the fact that GeometryPtr is a std::unique_ptr, but still. 

The best code styleguide is a consistent code base, so deciding and then globally changing makes the most sense, IMO.

P.
_______________________________________________
geos-devel mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/geos-devel

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

Re: GeometryPtr or ...

Martin Davis-3
As a counter-argument, it might be better to use the typedef, based on the principle of encapsulating implementation decisions.  Usual reasons:

* Reduces comprehension complexity 
* Makes implementation easier to change
* Indicates that this is an explicit design decision made in the library, rather than something decided by individual developers on a case-by-case basis

On Fri, Dec 7, 2018 at 6:40 AM Daniel Baston <[hidden email]> wrote:
I'd agree; I find that typedefs like GeometryPtr generally obfuscate things. Although one can guess, it's not immediately obvious if GeometryPtr means Geometry*, unique_ptr<Geometry>, shared_ptr<Geometry>, or something else.

Dan

On Thu, Dec 6, 2018 at 5:46 PM Paul Ramsey <[hidden email]> wrote:
std::unique_ptr<Geometry> ?

We've got a mishmash in the code base, what should it be?
As a learner arriving at the code base, std::unique_ptr<Geometry> would have been easier, since then the semantics of the thing are more immediately transparent then. After working with it for a while, that's less of an issue because I've internalized the fact that GeometryPtr is a std::unique_ptr, but still. 

The best code styleguide is a consistent code base, so deciding and then globally changing makes the most sense, IMO.

P.
_______________________________________________
geos-devel mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/geos-devel
_______________________________________________
geos-devel mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/geos-devel

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

Re: GeometryPtr or ...

Daniel Baston
I don't see what it encapsulates, though...it's not as though a GeometryPtr is appropriate for all cases where a pointer to a geometry is needed, and a user can manipulate a GeometryPtr without knowledge of the underlying type. A GeometryPtr is only suitable in a situation where a unique_ptr could be used, and a user must understand this to correctly use a GeometryPtr. (They would also need to refer to standard library documentation to understand what methods are available to a GeometryPtr, and how they are to be used.) I guess the typedef would allow us to switch to some other implementation of a unique pointer without a user being aware, but given that unique_ptr is now in the standard library it's hard to imagine a situation where that would happen.

Agree with Paul that consistency is probably the most important thing. Crude grepping shows ~700 usages of unique_ptr, ~200 usages of Geometry::Ptr, ~100 usages of GeometryPtr, but this misses unqualified uses of "Ptr" within classes that inherit from Geometry.

Dan

On Fri, Dec 7, 2018 at 12:08 PM Martin Davis <[hidden email]> wrote:
As a counter-argument, it might be better to use the typedef, based on the principle of encapsulating implementation decisions.  Usual reasons:

* Reduces comprehension complexity 
* Makes implementation easier to change
* Indicates that this is an explicit design decision made in the library, rather than something decided by individual developers on a case-by-case basis

On Fri, Dec 7, 2018 at 6:40 AM Daniel Baston <[hidden email]> wrote:
I'd agree; I find that typedefs like GeometryPtr generally obfuscate things. Although one can guess, it's not immediately obvious if GeometryPtr means Geometry*, unique_ptr<Geometry>, shared_ptr<Geometry>, or something else.

Dan

On Thu, Dec 6, 2018 at 5:46 PM Paul Ramsey <[hidden email]> wrote:
std::unique_ptr<Geometry> ?

We've got a mishmash in the code base, what should it be?
As a learner arriving at the code base, std::unique_ptr<Geometry> would have been easier, since then the semantics of the thing are more immediately transparent then. After working with it for a while, that's less of an issue because I've internalized the fact that GeometryPtr is a std::unique_ptr, but still. 

The best code styleguide is a consistent code base, so deciding and then globally changing makes the most sense, IMO.

P.
_______________________________________________
geos-devel mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/geos-devel
_______________________________________________
geos-devel mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/geos-devel
_______________________________________________
geos-devel mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/geos-devel

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

Re: GeometryPtr or ...

Martin Davis-3
Well, I don't have a strong preference on this (yet).  I do like code brevity, though.  So would probably opt for using GeomPtr instead of "GeometryPtr".  

And yes, users do have to understand what they are doing.  And should not run with scissors. :)  I thought perhaps defining a library typedef would mean that user can just follow a simpler pattern without feeling they need to grok everything about std:unique_ptr.

On Fri, Dec 7, 2018 at 9:29 AM Daniel Baston <[hidden email]> wrote:
I don't see what it encapsulates, though...it's not as though a GeometryPtr is appropriate for all cases where a pointer to a geometry is needed, and a user can manipulate a GeometryPtr without knowledge of the underlying type. A GeometryPtr is only suitable in a situation where a unique_ptr could be used, and a user must understand this to correctly use a GeometryPtr. (They would also need to refer to standard library documentation to understand what methods are available to a GeometryPtr, and how they are to be used.) I guess the typedef would allow us to switch to some other implementation of a unique pointer without a user being aware, but given that unique_ptr is now in the standard library it's hard to imagine a situation where that would happen.

Agree with Paul that consistency is probably the most important thing. Crude grepping shows ~700 usages of unique_ptr, ~200 usages of Geometry::Ptr, ~100 usages of GeometryPtr, but this misses unqualified uses of "Ptr" within classes that inherit from Geometry.

Dan

On Fri, Dec 7, 2018 at 12:08 PM Martin Davis <[hidden email]> wrote:
As a counter-argument, it might be better to use the typedef, based on the principle of encapsulating implementation decisions.  Usual reasons:

* Reduces comprehension complexity 
* Makes implementation easier to change
* Indicates that this is an explicit design decision made in the library, rather than something decided by individual developers on a case-by-case basis

On Fri, Dec 7, 2018 at 6:40 AM Daniel Baston <[hidden email]> wrote:
I'd agree; I find that typedefs like GeometryPtr generally obfuscate things. Although one can guess, it's not immediately obvious if GeometryPtr means Geometry*, unique_ptr<Geometry>, shared_ptr<Geometry>, or something else.

Dan

On Thu, Dec 6, 2018 at 5:46 PM Paul Ramsey <[hidden email]> wrote:
std::unique_ptr<Geometry> ?

We've got a mishmash in the code base, what should it be?
As a learner arriving at the code base, std::unique_ptr<Geometry> would have been easier, since then the semantics of the thing are more immediately transparent then. After working with it for a while, that's less of an issue because I've internalized the fact that GeometryPtr is a std::unique_ptr, but still. 

The best code styleguide is a consistent code base, so deciding and then globally changing makes the most sense, IMO.

P.
_______________________________________________
geos-devel mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/geos-devel
_______________________________________________
geos-devel mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/geos-devel
_______________________________________________
geos-devel mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/geos-devel
_______________________________________________
geos-devel mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/geos-devel

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

Re: GeometryPtr or ...

Daniel Baston
would probably opt for using GeomPtr instead of "GeometryPtr".  

Didn't think to look for that one! Looks like we've got ~500 of those, too.

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

Re: GeometryPtr or ...

Sandro Santilli-4
In reply to this post by Martin Davis-3
On Fri, Dec 07, 2018 at 09:08:01AM -0800, Martin Davis wrote:

> * Makes implementation easier to change

This was exactly the reason for starting to use that implementation.
Wasn't it an auto_ptr before scoped_ptr existed ?

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

Re: GeometryPtr or ...

Kurt Schwehr-2
My take... Using a typedef might make sense until std:: gets an implementation that is successfully battle tested and generally adopted.  std::auto_ptr miss the boat in many ways.  std::unique_ptr has staying power and most new C++ coders are expected to know and use it.  So I think hiding it behind a typedef is usually a hindrance to the reader.  I've tried typedefs with it several times and it generally hurt more than it helped.  And the cases where it seemed to help start going away with C++14 and C++17 IMHO.


On Sat, Dec 8, 2018 at 9:35 AM Sandro Santilli <[hidden email]> wrote:
On Fri, Dec 07, 2018 at 09:08:01AM -0800, Martin Davis wrote:

> * Makes implementation easier to change

This was exactly the reason for starting to use that implementation.
Wasn't it an auto_ptr before scoped_ptr existed ?

--strk;
_______________________________________________
geos-devel mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/geos-devel


--

_______________________________________________
geos-devel mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/geos-devel