<div dir="ltr"><div>I'm not sure moving the std::vector<SegmentString*> to shared pointers will be necessary, simply using std::vector<unique_ptr<SegmentString>> is probably sufficient. The "real problem" as you note is just that the contract isn't explicit anywhere. I think that the move the unique_ptr across other parts of the API avoided this chunk of code because it was and remains pretty gnarly. I did not want to refactor it because I'm dealing with enough mess just bringing in the new code. <br></div><div>A refactor is on my list now though, including some work to reduce some of the huge amount of copying that goes on under the covers, some of it unnecessary</div><div><br></div><div>In other news, I have identified and removed the memory leaks, everything I run through valgrind is now clean. So, onwards towards merging the branch to master, once I get CI green. <br></div><div><br></div><div>P.<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jul 24, 2020 at 8:00 AM Sandro Santilli <<a href="mailto:strk@kbt.io">strk@kbt.io</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Fri, Jul 24, 2020 at 12:07:44PM +0200, Sandro Santilli wrote:<br>
> <br>
> The ownership of those SegmentStrings is not documented in<br>
> NodedSegmentString.h, which would help. My impression is that<br>
> those segment strings should be shared pointers, to overcome this<br>
> long standing issue (it was a problem before snaprounding as well).<br>
<br>
More information. I think the primary problem with this API is that<br>
the ownership of SegmentString in Noder.h is not properly documented.<br>
<br>
Comments are even clearly expressing the confusion:<br>
<br>
     * Some Noders may add all these nodes to the input SegmentStrings;<br>
     * others may only add some or none at all.<br>
<br>
The methods defined for a Noder are:<br>
<br>
  virtual void computeNodes(std::vector<SegmentString*>* segStrings) = 0;<br>
  virtual std::vector<SegmentString*>* getNodedSubstrings() const = 0;<br>
<br>
What isn't clear is:<br>
<br>
  - Who owns the SegmentString passed to computeNodes ?<br>
  - Who owns the SegmentString returned by getNodedSubstrings ?<br>
  - Can the same SegmentString be present in both input and output<br>
    containers ?<br>
<br>
I think these questions should be answered with proper documentation<br>
in that class, and then we need to make sure all subclass implement<br>
the documented semantic properly.<br>
<br>
Using shared pointers of those SegmentStrings may help simplifying<br>
the implementations, with the downside of some more overhead. But<br>
other implementations could be made to work, as long as it is clear<br>
who is responsible for what...<br>
<br>
--strk;<br>
_______________________________________________<br>
geos-devel mailing list<br>
<a href="mailto:geos-devel@lists.osgeo.org" target="_blank">geos-devel@lists.osgeo.org</a><br>
<a href="https://lists.osgeo.org/mailman/listinfo/geos-devel" rel="noreferrer" target="_blank">https://lists.osgeo.org/mailman/listinfo/geos-devel</a></blockquote></div>