Add unit tests for TileCoordinate and TilePoint #6626
Conversation
|
@brunoabinader, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jfirebaugh, @ansis and @kkaefer to be potential reviewers. |
| @@ -22,8 +22,12 @@ | |||
| namespace mbgl { | |||
| namespace style { | |||
|
|
|||
| namespace { | |||
There was a problem hiding this comment.
Not necessary here; static data and functions are already non-visible beyond their translation unit.
| @@ -168,7 +172,8 @@ void Source::Impl::updateTiles(const UpdateParameters& parameters) { | |||
| parameters.debugOptions & MapDebugOptions::Collision }; | |||
|
|
|||
| for (auto& pair : tiles) { | |||
| pair.second->setPlacementConfig(config); | |||
| auto& tile = pair.second; | |||
| tile->setPlacementConfig(config); | |||
There was a problem hiding this comment.
Why introduce a temporary?
There was a problem hiding this comment.
Intention is to improve readability (tile vs. pair.second).
If that's agreeable, we can move the object scope out of the for-loop so it only gets updated instead of created/deleted on each pass.
There was a problem hiding this comment.
I prefer the original -- pair.second->setPlacementConfig(config); -- over introducing any temporary variables. It's clear from the previous line that you're iterating over tiles.
| @@ -25,8 +27,22 @@ namespace style { | |||
| class Layer; | |||
| } // namespace style | |||
|
|
|||
| using TilePoint = Point<int16_t>; | |||
There was a problem hiding this comment.
We have a typedef for this already, currently spelled GeometryCoordinate. I think TilePoint is better, but let's wait to introduce it until we can do a universal rename and at the same time remove GeometryCoordinates and GeometryCollection in favor of geometry.hpp types.
| class Tile : private util::noncopyable { | ||
| public: | ||
| static TilePoint pointFromTileCoordinate(const UnwrappedTileID& tileID, const TileCoordinatePoint& p) { |
There was a problem hiding this comment.
This is more closely related to to TileCoordinate than Tile -- let's move it there.
| @@ -514,3 +520,62 @@ TEST(Transform, DefaultTransform) { | |||
| ASSERT_FALSE(transform.resize({{ max, max }})); | |||
| testConversions(nullIsland, center); | |||
| } | |||
|
|
|||
| TEST(Transform, TileCoordinate) | |||
There was a problem hiding this comment.
Let's put all the TileCoordinate-related tests -- the ones here and in tile_point.test.cpp -- in tile_coordinate.test.cpp.
I've been campaigning to get rid of TileCoordinate for awhile (1, 2, 3) -- keeping it and its tests each confined to a single file at least limits the spread as much as possible.
brunoabinader
force-pushed
the
6626-tile-coordinate-utests
branch
2 times, most recently
from
9d64b3c to
2111b77
Compare
brunoabinader
force-pushed
the
6626-tile-coordinate-utests
branch
from
2111b77 to
d16eb20
Compare
|
Addressed @jfirebaugh review comments. |
brunoabinader
force-pushed
the
6626-tile-coordinate-utests
branch
4 times, most recently
from
4bf0469 to
f57f992
Compare
| mapbox::geometry::box<double> box = mapbox::geometry::envelope(queryGeometry); | ||
|
|
||
| for (const auto& tilePtr : renderTiles) { | ||
| const RenderTile& tile = tilePtr.second; |
There was a problem hiding this comment.
I'd keep this; it's pair isn't a name that says anything.
brunoabinader
force-pushed
the
6626-tile-coordinate-utests
branch
from
f57f992 to
76dc593
Compare
brunoabinader
force-pushed
the
6626-tile-coordinate-utests
branch
from
76dc593 to
5861bf3
Compare
brunoabinader
force-pushed
the
6626-tile-coordinate-utests
branch
from
5861bf3 to
31c5aa8
Compare
Previously part of #6553.