change QgsAbstractGeometryV2::coordinateSequence() to return a

[jef-n]

implicitly shared copy of an internal cache instead of recreating the coordinate sequence again and again.

Improves performance of the nodetool on large features a lot (refs #13963)

Also introduce Qgs(Coordinate|Ring|Point)SequenceV2 typedefs.

[nyalldawson]

Very nice!! I've also had concerns about the expense of calling these methods.

Two questions:

• is the API break ok on this case? (No personal objections from me)
• how did you identify where to clear the cache? If it was done by looking for where the bounding box cache was cleared alone then I suspect there's missing calls. When writing the tests for point/line string/polygon I fixed many places where these calls were missing, but haven't been able to do that for the other geometry types yet.

Otherwise, big +1 from me

[jef-n]

is the API break ok on this case? (No personal objections from me)

Is there any? The typedef shouldn't break anything and that's what changes in QgsGeometry. The other changes are only the V2 geometry classes and those are not part of the stable API, are they? Or did I break anything else?

how did you identify where to clear the cache? If it was done by looking for where the bounding box cache was cleared alone then I suspect there's missing calls. When writing the tests for point/line string/polygon I fixed many places where these calls were missing, but haven't been able to do that for the other geometry types yet.

Yes, running the tests pointed at a missing reset in QgsPolygonV2::setExteriorRing. That's fixed there.

But I also suspect that there is more. If there hadn't been that mBoundingBox, I would probably had given up right away. My first approach was to add variants of the methods operating on a coordinate sequence with a version that operates on a passed in coordinate sequence. But there always was another method that retrieve a coordinate sequence - so that didn't really help, caused more clutter and API changes.

[nyalldawson]

Is there any? The typedef shouldn't break anything and that's what changes in QgsGeometry. The other changes are only the V2 geometry classes and those are not part of the stable API, are they? Or did I break anything else?

I'm not sure... but my gut feeling was that these classes weren't fully exposed to the python bindings pre 2.14 anyway, so we should be safe. Also, who would be using this class outside of core? In #2824 I've made this method protected. So no objections here from me!

Anyway, #2824 should help improve (slightly) the cache invalidation situation and make it easier to implement stuff like this.

[jef-n]

updated after merge of #2824

[nyalldawson]

Looks like you missed adding a clear of the cached coordinate string to the clearCache implementations?