Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Commit

Permalink
bring back const-ness of abstract GeometryTileLayer
Browse files Browse the repository at this point in the history
  • Loading branch information
@incanus
incanus committed Mar 15, 2015
1 parent fc0d814 commit fbd5511
Show file tree
Hide file tree
Showing 6 changed files with 7 additions and 6 deletions.
3 changes: 2 additions & 1 deletion src/mbgl/map/annotation.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ std::vector<uint32_t> AnnotationManager::addPointAnnotations(std::vector<LatLng>
auto tile_it = annotationTiles.find(tileID);
if (tile_it != annotationTiles.end()) {
auto layer = tile_it->second->getLayer("annotations");
auto liveLayer = std::static_pointer_cast<LiveTileLayer>(layer);
auto mutableLayer = std::const_pointer_cast<GeometryTileLayer>(layer);
auto liveLayer = std::static_pointer_cast<LiveTileLayer>(mutableLayer);
liveLayer->addFeature(feature);
} else {
util::ptr<LiveTileLayer> layer = std::make_shared<LiveTileLayer>();
Expand Down
2 changes: 1 addition & 1 deletion src/mbgl/map/geometry_tile.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class GeometryTileLayer : private util::noncopyable {

class GeometryTile : private util::noncopyable {
public:
virtual util::ptr<GeometryTileLayer> getLayer(const std::string&) const = 0;
virtual util::ptr<const GeometryTileLayer> getLayer(const std::string&) const = 0;
};

class GeometryTileFeatureExtractor {
Expand Down
2 changes: 1 addition & 1 deletion src/mbgl/map/live_tile.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ void LiveTile::convert() {
layers.emplace("annotations", std::make_shared<LiveTileLayer>(tile->features));
}

util::ptr<GeometryTileLayer> LiveTile::getLayer(const std::string& name) const {
util::ptr<const GeometryTileLayer> LiveTile::getLayer(const std::string& name) const {
auto layer_it = layers.find(name);
if (layer_it != layers.end()) {
return layer_it->second;
Expand Down
2 changes: 1 addition & 1 deletion src/mbgl/map/live_tile.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class LiveTile : public GeometryTile, private util::noncopyable {
LiveTile(TTile*);

void addLayer(const std::string&, util::ptr<LiveTileLayer>);
util::ptr<GeometryTileLayer> getLayer(const std::string&) const override;
util::ptr<const GeometryTileLayer> getLayer(const std::string&) const override;
bool operator()(const LiveTile&) const { return layers.size() > 0; }

private:
Expand Down
2 changes: 1 addition & 1 deletion src/mbgl/map/vector_tile.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ VectorTile::VectorTile(pbf tile_pbf) {
}
}

util::ptr<GeometryTileLayer> VectorTile::getLayer(const std::string& name) const {
util::ptr<const GeometryTileLayer> VectorTile::getLayer(const std::string& name) const {
auto layer_it = layers.find(name);
if (layer_it != layers.end()) {
return layer_it->second;
Expand Down
2 changes: 1 addition & 1 deletion src/mbgl/map/vector_tile.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class VectorTile : public GeometryTile {
public:
VectorTile(pbf);

util::ptr<GeometryTileLayer> getLayer(const std::string&) const override;
util::ptr<const GeometryTileLayer> getLayer(const std::string&) const override;

private:
std::unordered_map<std::string, util::ptr<GeometryTileLayer>> layers;
Expand Down

3 comments on commit fbd5511

@incanus
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would love an eyeball on this @jfirebaugh when you have a moment. Because I am wanting to mutate LiveTileLayer (the whole point of the class), but we want to default to returning const GeometryTileLayer in getLayer(), I'm doing some pointer casting.

@jfirebaugh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, these pointer casts are a code smell. What you want here is a covariant return type for getLayer, where the abstract base is declared as returning GeometryTileLayer pointers, but LiveTile is declared as returning a LiveTileLayer pointer. C++ doesn't have a way to do this with smart pointers though. Your best bet is to define a separate getter that returns the desired type. See http://stackoverflow.com/questions/196733/how-can-i-use-covariant-return-types-with-smart-pointers

With regards to constness, GeometryTileLayer's interface is const, so it might make sense to go with a util::ptr<GeometryTileLayer> return type for simplicity's sake. The thing const does is remind you to think about thread safety if you cast it away. Which thread is writing LiveTileLayers and which thread(s) are reading them?

@incanus
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, these pointer casts are a code smell.

Yep, felt like it. Thanks for the pointers (heh); I will shore this up.

it might make sense to go with a util::ptr<GeometryTileLayer>

I am game for this approach right now. When we get into possible shape annotation style geometry mutation in future, will give it a re-think.

Please sign in to comment.