[WIP] replace mbgl::variant with std::variant #991

bencsikandrei · 2023-04-07T20:08:48Z

This is part 1/n of a series of commits (or PRs?) towards solving: #893

My idea for this PR is to not aim at fully resolving this issue, as I believe it would be too much for one PR, but rather make small increments towards that goal (solve the simple things first, etc...)

My decision was to split into at a couple commits since I have some concerns regarding replacing some of the functionality of mbgl::variant with std::variant equivalents (.match and some of its operator== semantics come to mind)

Please let me know if my decision suits what you have envisioned for this task

Thanks!

bencsikandrei · 2023-04-07T20:10:01Z

expression-test/expression_test_parser.cpp

@@ -407,12 +407,12 @@ Ignores parseExpressionIgnores() {
 std::optional<TestData> parseTestData(const filesystem::path& path) {
    TestData data;
    auto maybeJson = readJson(path.string());
-    if (!maybeJson.is<JSDocument>()) { // NOLINT
+    if (!std::holds_alternative<JSDocument>(maybeJson)) { // NOLINT


I'm not sure what was linted here, if someone could help me with that one I'd happily check and maybe remove the NOLINT comment

I'll do some searching in the meanting to try to figure out what analyzer I should run to test this

Could you try removing it? If the CI passes we can remove it.

I think Mapbox used some kind of linter in the past, but we are not (yet).

louwers

Looks great overall but I would like to know why the Overload helper is needed.

louwers · 2023-04-11T08:05:56Z

include/mbgl/util/overloaded.hpp

+namespace util
+{
+
+template <typename... Ts>


Everything in include will become part of our public API. Is this intended? Can you add some triple slash comments to indicate what this template struct is for?

It may make sense to make it internal, in that case you can move it to /src.

Some comments (and a link to cppreference) would still be a good idea in that case.

agreed! I believe there's not need for it to be public but I have to check

done, I moved it to src/mbgl/util

bencsikandrei · 2023-04-11T08:35:48Z

Looks great overall but I would like to know why the Overload helper is needed.

the API for std::visit is quite different from the one in mapbox::variant::match, in that it favors applying the same visitor to multiple variant objects (via variadic template arguments) as opposed to multiple callables on the same variant object

the API is as follows:
template <class Visitor, class... Variants> constexpr auto visit( Visitor&& vis, Variants&&... vars );

which means we've got to pass 1 visitor for N (which could also be 0) variants

in order to obtain the same behavior as mapbox::variant::match (which AFAIK takes a variadic template argument pack for multiple function like objects) we need to pass some sort of function like object which contains all the visit functions we need

one way to do so is to manually create a visitor with multiple overloads for operator()(...)

e.g.: struct Visitor { void operator()(int); void operator()(double); ..}; which we could apply to a variant<int, double>, but that's tedious and we already have the lambdas from the old calls to match so here's where the Overloaded thing comes in

it effectively inherits from a bunch of lambdas (so it has all their operator()(...)) in a single place (it, unfortunately, also needs a deduction guide for c++17 to allow for {..} initialization from the lambdas)

maybe I made the explanation way too long sorry for that!

here's where I took it from, by the way: https://en.cppreference.com/w/cpp/utility/variant/visit

louwers · 2023-04-18T23:08:27Z

Please click re-request review when you are ready! Thanks 🙂

github-actions · 2023-04-27T21:31:14Z

Test results at https://maplibre-native-test-artifacts.s3.eu-central-1.amazonaws.com/4824605626-linux-gcc8-release-style.html

github-actions · 2023-04-29T20:43:06Z

Test results at https://maplibre-native-test-artifacts.s3.eu-central-1.amazonaws.com/4840695425-linux-gcc8-release-style.html

…mpoud_expression

rename `Overload` to `Overloaded`

bencsikandrei · 2023-05-09T19:56:52Z

Hi!

I've been working on this on and off but have struggled with a couple things in the public headers (notably changes involving some stuff that mbgl::variant and std::variant handle differently - mostly related to operator== and co.).

In any case since this has been a long PR and has been lingering I've had to rebase several times.

I believe the changes that exist now (that don't touch public headers) could be merged in a first step.

Then we can move to the other (fewer) changes to the public headers and a couple platform things. Those will, hopefully, be faster to do than this one.

I'd rather have everything done in one PR but I've not had the time to figure out what happens to a couple headers when I changed to std::variant (the compiler cries for help with errors).

Keeping this up to date will become increasingly harder if it lingers and I'd rather at least make a small step.

What do you think?

bencsikandrei · 2023-05-09T20:09:27Z

I also have 0 idea what this wants from me:

`/platform/macos/src/MLNMapViewDelegate.h:88:63: error: expected a type

(BOOL)mapView:(MLNMapView *)mapView shouldChangeFromCamera:(MLNMapCamera *)oldCamera toCamera:(MLNMapCamera *)newCamera;
^
./platform/macos/src/MLNMapViewDelegate.h:88:98: error: expected a type
(BOOL)mapView:(MLNMapView *)mapView shouldChangeFromCamera:(MLNMapCamera *)oldCamera toCamera:(MLNMapCamera *)newCamera;
^
./platform/macos/src/MLNMapViewDelegate.h:182:62: error: expected a type
(void)mapView:(MLNMapView *)mapView didFinishLoadingStyle:(MLNStyle *)style;
^
./platform/macos/src/MLNMapViewDelegate.h:212:88: error: no type or protocol named 'MLNAnnotation'
(nullable MLNAnnotationImage *)mapView:(MLNMapView *)mapView imageForAnnotation:(id )annotation;
^
./platform/macos/src/MLNMapViewDelegate.h:244:4: error: expected a type
(NSColor *)mapView:(MLNMapView *)mapView strokeColorForShapeAnnotation:(MLNShape *)annotation;
^
./platform/macos/src/MLNMapViewDelegate.h:259:4: error: expected a type
(NSColor *)mapView:(MLNMapView *)mapView fillColorForPolygonAnnotation:(MLNPolygon *)annotation;
^
./platform/macos/src/MLNMapViewDelegate.h:295:64: error: no type or protocol named 'MLNAnnotation'
(void)mapView:(MLNMapView *)mapView didSelectAnnotation:(id )annotation;
^
./platform/macos/src/MLNMapViewDelegate.h:305:66: error: no type or protocol named 'MLNAnnotation'
(void)mapView:(MLNMapView *)mapView didDeselectAnnotation:(id )annotation;
^
./platform/macos/src/MLNMapViewDelegate.h:330:69: error: no type or protocol named 'MLNAnnotation'
(BOOL)mapView:(MLNMapView *)mapView annotationCanShowCallout:(id )annotation;
^
./platform/macos/src/MLNMapViewDelegate.h:348:13: error: expected a type
(nullable NSViewController *)mapView:(MLNMapView *)mapView calloutViewControllerForAnnotation:(id )annotation;
^
./platform/macos/src/MLNMapViewDelegate.h:348:102: error: no type or protocol named 'MLNAnnotation'
(nullable NSViewController *)mapView:(MLNMapView *)mapView calloutViewControllerForAnnotation:(id )annotation;
^
:0: error: failed to import bridging header 'platform/macos/src/MLNMapViewDelegate.h'`

If someone could help me with iOS that would be amazing!

github-actions · 2023-05-09T21:38:26Z

Render test results at https://maplibre-native-test-artifacts.s3.eu-central-1.amazonaws.com/4930719458-linux-gcc8-release-style.html

louwers · 2024-03-03T21:09:35Z

@bencsikandrei Sorry for never replying, I am afraid I don't understand that error either.

Let's see if we can get this in a mergable state.

bencsikandrei commented Apr 7, 2023

View reviewed changes

bencsikandrei force-pushed the 893-replace-custom-variant-with-std-variant-1 branch from 036031d to 282c86f Compare April 10, 2023 19:31

bencsikandrei marked this pull request as draft April 10, 2023 19:32

louwers added the cpp-core label Apr 11, 2023

louwers requested changes Apr 11, 2023

View reviewed changes

louwers assigned bencsikandrei and louwers and unassigned bencsikandrei Apr 11, 2023

bencsikandrei force-pushed the 893-replace-custom-variant-with-std-variant-1 branch from aac58c8 to bf34bd5 Compare April 17, 2023 19:57

bencsikandrei changed the title ~~[WIP] replace mbgl::variant with std::variant in expression_test_parser~~ [WIP] replace mbgl::variant with std::variant Apr 18, 2023

bencsikandrei force-pushed the 893-replace-custom-variant-with-std-variant-1 branch 4 times, most recently from b15e082 to 6f10640 Compare April 27, 2023 20:09

bencsikandrei force-pushed the 893-replace-custom-variant-with-std-variant-1 branch from 6f10640 to 7b047e4 Compare April 29, 2023 19:05

bencsikandrei added 9 commits May 9, 2023 22:08

replace mbgl::variant with std::variant in expression_test_parser

980a311

replace mbgl::variant with std::variant in update_renderables_test

0db2e32

replace mbgl::variant with std::variant - raster|vector_source and co…

3d15f63

…mpoud_expression

mbgl::variant -> std::variant style_diff | possibily evaluated prop

255c6f7

move overloaded.hpp to src/util

f88ab11

rename `Overload` to `Overloaded`

mbgl::variant -> std::variant

5b4d719

mbgl::variant -> std::variant in line_atlas

8fb2ae5

mbgl::variant -> std::variant in render_test

589ad19

mbgl::variant -> std::variant style/sources

7a70a97

bencsikandrei force-pushed the 893-replace-custom-variant-with-std-variant-1 branch from 7b047e4 to 7a70a97 Compare May 9, 2023 19:51

bencsikandrei requested a review from louwers May 9, 2023 19:52

fix iOS constructor -> std::variant

89832ab

bencsikandrei force-pushed the 893-replace-custom-variant-with-std-variant-1 branch from 0f7c1ab to 89832ab Compare May 9, 2023 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] replace mbgl::variant with std::variant #991

[WIP] replace mbgl::variant with std::variant #991

bencsikandrei commented Apr 7, 2023 •

edited

Loading

bencsikandrei Apr 7, 2023

louwers Apr 11, 2023

louwers left a comment

louwers Apr 11, 2023

bencsikandrei Apr 11, 2023

louwers Apr 11, 2023 •

edited

Loading

bencsikandrei Apr 11, 2023

bencsikandrei Apr 14, 2023

bencsikandrei commented Apr 11, 2023 •

edited

Loading

louwers commented Apr 18, 2023

github-actions bot commented Apr 27, 2023

github-actions bot commented Apr 29, 2023

bencsikandrei commented May 9, 2023

bencsikandrei commented May 9, 2023

github-actions bot commented May 9, 2023

louwers commented Mar 3, 2024

[WIP] replace mbgl::variant with std::variant #991

Are you sure you want to change the base?

[WIP] replace mbgl::variant with std::variant #991

Conversation

bencsikandrei commented Apr 7, 2023 • edited Loading

bencsikandrei Apr 7, 2023

Choose a reason for hiding this comment

louwers Apr 11, 2023

Choose a reason for hiding this comment

louwers left a comment

Choose a reason for hiding this comment

louwers Apr 11, 2023

Choose a reason for hiding this comment

bencsikandrei Apr 11, 2023

Choose a reason for hiding this comment

louwers Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

bencsikandrei Apr 11, 2023

Choose a reason for hiding this comment

bencsikandrei Apr 14, 2023

Choose a reason for hiding this comment

bencsikandrei commented Apr 11, 2023 • edited Loading

louwers commented Apr 18, 2023

github-actions bot commented Apr 27, 2023

github-actions bot commented Apr 29, 2023

bencsikandrei commented May 9, 2023

bencsikandrei commented May 9, 2023

github-actions bot commented May 9, 2023

louwers commented Mar 3, 2024

bencsikandrei commented Apr 7, 2023 •

edited

Loading

louwers Apr 11, 2023 •

edited

Loading

bencsikandrei commented Apr 11, 2023 •

edited

Loading