Fix private header includes #8147
Conversation
|
@jfirebaugh, thanks for your PR! By analyzing this pull request, we identified @tmpsantos, @ansis and @tmcw to be potential reviewers. |
jfirebaugh
force-pushed
the
private-src
branch
from
51f6f78
to
ca968f0
Compare
cmake/core-files.cmake
Outdated
| include/mbgl/util/logging.hpp | ||
| include/mbgl/util/noncopyable.hpp | ||
| include/mbgl/util/optional.hpp | ||
| include/mbgl/util/platform.hpp | ||
| include/mbgl/util/projection.hpp | ||
| include/mbgl/util/range.hpp | ||
| include/mbgl/util/rapidjson.hpp |
There was a problem hiding this comment.
We've kept this private because it requires having the RapidJSON headers present. What's the reason for making this public?
There was a problem hiding this comment.
It's used by Qt SDK bindings, specifically
. @tmpsantos @brunoabinader do you see a way to keep it private?
There was a problem hiding this comment.
We could make Result<GeoJSON> convertGeoJSON(const QVariant& value) to use an overloaded version of convertGeoJSON internally that receives a std::string as input. This overloaded version could then use rapidjson to parse this and reside outside of Qt scope.
The same could be made in e.g. https://github.com/mapbox/mapbox-gl-native/blob/master/platform/android/src/style/conversion/geojson.hpp.
There was a problem hiding this comment.
Done in f6f826d0b5d3948422b764272f388d6487559c87 e61214eaec702547a010ca6c2ca7df47ef60be02.
| @@ -9,7 +9,7 @@ | |||
| #include <mbgl/style/conversion/source.hpp> | |||
| #include <mbgl/style/conversion/layer.hpp> | |||
| #include <mbgl/style/conversion/filter.hpp> | |||
| #include <mbgl/sprite/sprite_image.cpp> | |||
| #include <mbgl/sprite/sprite_image.hpp> | |||
There was a problem hiding this comment.
How did this ever work without duplicate symbol errors?
There was a problem hiding this comment.
Yeah, right? I don't know.
| src/mbgl/gl/vertex_array.cpp | ||
| src/mbgl/gl/vertex_array.hpp | ||
| src/mbgl/gl/vertex_buffer.hpp |
There was a problem hiding this comment.
Why do we make them public? They only seem to be used within the rendering subsystem, which isn't exposed.
jfirebaugh
force-pushed
the
private-src
branch
3 times, most recently
from
31ec3d0
to
c478e95
Compare
brunoabinader
force-pushed
the
private-src
branch
from
f6f826d
to
e61214e
Compare
Map constructor takes Scheduler&, and consumers are expected to define an implementation. Therefore the interface must be public.
These should be re-privatized after refactoring to remove the public dependency on context.hpp and extension.hpp.
jfirebaugh
force-pushed
the
private-src
branch
from
e61214e
to
af614cb
Compare
|
Nice, thanks @brunoabinader. Ok, ready for a final review here. |
|
@jfirebaugh please see #8216 for an alternative that includes all of the commits in this PR, except for the one making the GL headers public. |
|
⇢ #8216 |
Move some includes to public include directory, avoid others.
Fixes #4647.