<div dir="ltr"><div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial">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.</div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><br></div><div style="text-align:start;text-indent:0px;text-decoration-style:initial;text-decoration-color:initial">You may want to consider some singletons.<br></div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><br></div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial">Some thoughts on the code since it's out there...</div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><br></div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><br></div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial">SingleCRS has <span style="color:rgb(36,41,46);font-family:SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;font-size:12px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:pre;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">PROJ_OPAQUE_PRIVATE_DATA, so why do the derived classes</span> like GeodeticCRS also have PROJ_OPAQUE_PRIVATE_DATA?</div><div><br></div><div><br></div><div>Two layout requests:</div><div><br></div><div>1. Can you switch to public, protected, private order in classes. That usually more closely matches what a reader is likely needing to know.</div><div><br></div>2. Please try to avoid usings namespace polluting usings like this:</div><div><br></div><div>using namespace common;</div><div>using namespace datum;</div><div>using namespace cs;</div><div>using namespace operation;</div><div>using namespace util;</div><div><br></div><div>I find it easier to follow when I see:</div><div><br></div><div>using ::osgeo::proj::datum::Datum; // etc.</div><div><br></div><div>That way you don't have to worry about name collisions or other surprises.</div><div>And it provides a concise list to the reader.</div><div><br></div><div>Or just use the namespace when you use the particular... e.g.</div><div><br></div><div>const auto cs_helper = std::make_unique<cs::Helper>(foo, bar);</div><div><br></div><div><br></div><div>No need for virtual and override. virtual is redundant</div><div><br></div><div>virtual const GeodeticReferenceFrame *datum() const override;<br></div><div><br></div><div>Should just be:</div><div><br></div><div>const GeodeticReferenceFrame *datum() const override;<br></div><div><br></div><div><br></div><div>With these 2 methods, why is one returning a const ptr and the other a const ref?</div><div><br></div><div><div>virtual const GeodeticReferenceFrame *datum() const override;</div><div>const GeodeticCS &geodeticCoordinateSystem() const;<br></div></div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, May 27, 2018 at 8:35 AM, Even Rouault <span dir="ltr"><<a href="mailto:even.rouault@spatialys.com" target="_blank">even.rouault@spatialys.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
I just wanted to give a very early preview of my prototyping of the ISO <br>
191111:2018 class hiearchy, so we can discuss some coding practices (so <br>
non-"cosmetic" aspects of coding), especially about fundamental conventions <br>
deeply impacting the API, particularly regarding object ownership.<br>
<br>
This is not material ready for detailed review: there are lots of TODOs, no <br>
docs, mostly untested, doesn't do anything really useful yet, etc.<br>
<br>
I've uploaded the Doxygen doc generated from the work in progress. Two useful <br>
links to have an overview:<br>
<br>
Diagram of class hiearchy:<br>
<a href="http://even.rouault.free.fr/proj_cpp_api/html/inherits.html" rel="noreferrer" target="_blank">http://even.rouault.free.fr/<wbr>proj_cpp_api/html/inherits.<wbr>html</a><br>
<br>
Namespaces/packages and classes<br>
<a href="http://even.rouault.free.fr/proj_cpp_api/html/annotated.html" rel="noreferrer" target="_blank">http://even.rouault.free.fr/<wbr>proj_cpp_api/html/annotated.<wbr>html</a><br>
<br>
Example of the CRS package (formatted with clang-format):<br>
<a href="https://github.com/rouault/proj.4/blob/iso19111/src/crs.hh" rel="noreferrer" target="_blank">https://github.com/rouault/<wbr>proj.4/blob/iso19111/src/crs.<wbr>hh</a><br>
<a href="https://github.com/rouault/proj.4/blob/iso19111/src/crs.cpp" rel="noreferrer" target="_blank">https://github.com/rouault/<wbr>proj.4/blob/iso19111/src/crs.<wbr>cpp</a><br>
<br>
Test program:<br>
<a href="https://github.com/rouault/proj.4/blob/iso19111/test/test_cpp.cpp" rel="noreferrer" target="_blank">https://github.com/rouault/<wbr>proj.4/blob/iso19111/test/<wbr>test_cpp.cpp</a><br>
<br>
A few principles I've adopted:<br>
- done in a way such that memory leaks cannot happen (internal use of <br>
unique_ptr)<br>
- all private members are strictly hidden in the header file, by delegating to <br>
a internal private struct in the .cpp file. This way we can add private <br>
members without impacting the ABI<br>
- from the GeoAPI design, I've kept the apparent design constraint of having <br>
immutable objects once constructed.<br>
- the getters generally return a pointer or const reference to the internal <br>
structure members. Which means that those objects must not be used after their <br>
belonging object has been destroyed<br>
<br>
~~~~~~<br>
<br>
There are different ways that could be used for object life-cycle management:<br>
<br>
1) the one I took in this first sketch: we pass objects by const reference, <br>
and methods/constructors that need to keep them, create a copy.<br>
<br>
For example:<br>
<br>
GeographicCRS createGeographicCRS(const PropertyMap& properties,<br>
const GeodeticReferenceFrame& datum,<br>
const EllipsoidalCS& cs);<br>
<br>
will construct a GeographicCRS object that will duplicate datum and cs <br>
internally.<br>
<br>
2) another one we could imagine: use shared pointers<br>
<br>
The above would become:<br>
<br>
std::shared_ptr<GeographicCRS><br>
createGeographicCRS(const PropertyMap& properties,<br>
std::shared_ptr<<wbr>GeodeticReferenceFrame> datum,<br>
std::shared_ptr<EllipsoidalCS> cs);<br>
<br>
3) have ad-hoc conventions regarding pointer ownership. But we probably want <br>
to avoid this as this is error-prone for API users (incluing PROJ internal <br>
use)<br>
<br>
~~~~~~<br>
<br>
Comparing the 2 first approaches:<br>
<br>
Copying approach:<br>
- pros: no null pointer checking needed when acception a const& argument<br>
- cons:<br>
* can involve a lot of copying around when building complex object <br>
hiearchies<br>
* internally involves a kind of hack ( clone() method ) so that the <br>
duplication correctly clones instances of derived classes. To be clear: let's <br>
consider the situation where you have a constructor that accepts a Base& or <br>
Base* argument, that you need to store as a class member. If you call it with <br>
a Derived object, you want a instance of Derived to be cloned, not its Base-<br>
only part.<br>
<br>
shared_ptr approach:<br>
- pros:<br>
* reduced copying, avoid the cloning hack<br>
* we could return shared_ptr objects in getters, making them super safe to <br>
use (the return can be used after the belonging object has been destroyed).<br>
- cons:<br>
* null-pointer situations can happen and must be checked / documented<br>
<br>
My feeling is that the shared_ptr approach could be better, although CRS <br>
instanciation is probably not critical in most use cases. Of course shared_ptr <br>
aren't completely free performance-wise, but I feel this would be more <br>
efficient than deep copying of complex objects.<br>
<br>
And, to save explicit null checking in situations where we don't want null <br>
pointers, we could wrap the shared_ptr in a nn<> class [1] that guarantees <br>
that the hold pointer is not null.<br>
<br>
We would then have:<br>
GeographicCRS_nnshptr<br>
createGeographicCRS(const PropertyMap& properties,<br>
GeodeticReferenceFrame_nnshptr datum,<br>
EllipsoidalCS_nnshptr cs);<br>
<br>
with using xxxx_nnshptr = nn_shared_ptr<xxxx>;<br>
<br>
Thoughts ?<br>
<br>
Even<br>
<br>
[1] <a href="https://github.com/dropbox/nn/blob/master/nn.hpp" rel="noreferrer" target="_blank">https://github.com/dropbox/nn/<wbr>blob/master/nn.hpp</a><br>
<span class="gmail-HOEnZb"><font color="#888888"><br>
-- <br>
Spatialys - Geospatial professional services<br>
<a href="http://www.spatialys.com" rel="noreferrer" target="_blank">http://www.spatialys.com</a><br>
______________________________<wbr>_________________<br>
Proj mailing list<br>
<a href="mailto:Proj@lists.maptools.org">Proj@lists.maptools.org</a><br>
<a href="http://lists.maptools.org/mailman/listinfo/proj" rel="noreferrer" target="_blank">http://lists.maptools.org/<wbr>mailman/listinfo/proj</a><br>
</font></span></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature">--<div><a href="http://schwehr.org" target="_blank">http://schwehr.org</a></div></div>
</div>