[ios, macos] Use std::vector for MGLMultiPoint coordinate storage

[boundsj]

Fixes #6580

cc @incanus @1ec5

[mention-bot]

@boundsj, thanks for your PR! By analyzing the history of the files in this pull request, we identified @incanus, @1ec5 and @jfirebaugh to be potential reviewers.

[boundsj]

This won't actually work in practice without #6888 😬

[1ec5]

We don’t need to maintain a _count ivar as parallel state. Instead, implement -count as _coordinates.size().

[1ec5]

You can populate _coordinates in one go with some pointer arithmetic:

_coordinates = { coords, coords + count };

or:

_coordinates.assign(coords, coords + count);

[1ec5]

Structs like CLLocationCoordinate2D are passed by value, so there’s no need to explicitly copy the struct below. Just return this value (or _coordinates.front()).

[1ec5]

While you’re here, can you change this to an NSAssert() so we get better diagnostics?

[1ec5]

Replace this whole for loop with:

std::copy(_coordinates.begin() + range.location, _coordinates.begin() + NSMaxRange(range), coords);

(Note the use of begin() instead of end() in the second parameter.)

[1ec5]

If the developer tries to replace no coordinates, this method should be a no-op instead of raising an exception. That may help to minimize the number of special cases the developer would need to implement.

[1ec5]

It’s pretty meaningless to have a multipoint of zero points. In fact, we assert as much below. So we should detect that case early, in here, asserting if count is zero.

[1ec5]

The way this method was implemented in the first place makes it possible to replace a segment at the end of the multipoint while appending. This is an antipattern that tends to mask off-by-one errors in client code. Methods such as -[NSMutableData replaceBytesInRange:withBytes:] raise an NSRangeException if the range exceeds the bounds of the receiver. I think this class should do likewise.

That would allow us to simplify this entire method to:

std::copy(coords, coords + range.length, _coordinates.begin() + range.location);

Plus an exception if NSMaxRange(range) > _coordinates.size() and the KVO guards. We’d need a separate -appendCoordinates:count::

_coordinates.insert(_coordinates.end(), coords, coords + count);

In the unlikely event that the developer really does need to replace the tail end while appending, they can do it in two steps. With #6583, we’ll introduce -replaceCoordinatesInRange:withCoordinates:count: to make it doable in one step:

_coordinates.erase(_coordinates.begin() + range.location, _coordinates.begin() + NSMaxRange(range));
_coordinates.insert(_coordinates.begin() + range.location, coords, coords + count);

(This is the most naïve approach; there are more performant approaches.)

[1ec5]

Even though MGLPointCollection is immutable, it would be great if we could migrate it over to std::vector too for consistency.

[1ec5]

This appends only the first coordinate in the passed-in array. To append the entire array:

_coordinates.insert(_coordinates.end(), coords, coords + count);

[1ec5]

This will call -pointCount on every iteration. Use C++ fast enumeration instead:

for (auto coordinate : _coordinates) {
    …
}