Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use std::variant instead of mapbox::util::variant #6903

Merged
merged 45 commits into from
May 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
4f46705
wip
SiarheiFedartsou May 25, 2024
ad990aa
wip
SiarheiFedartsou May 25, 2024
1f6e4ff
wip
SiarheiFedartsou May 25, 2024
5ce6b34
wip
SiarheiFedartsou May 25, 2024
e917e31
wip
SiarheiFedartsou May 25, 2024
423154f
wip
SiarheiFedartsou May 25, 2024
67941db
wip
SiarheiFedartsou May 25, 2024
6078a61
wip
SiarheiFedartsou May 25, 2024
b311c2a
Remove third_party/variant
SiarheiFedartsou May 25, 2024
076ce82
wip
SiarheiFedartsou May 25, 2024
12c662c
wip
SiarheiFedartsou May 25, 2024
42e9d5b
wip
SiarheiFedartsou May 25, 2024
d932509
wip
SiarheiFedartsou May 25, 2024
08b505a
wip
SiarheiFedartsou May 25, 2024
bb0ab44
wip
SiarheiFedartsou May 25, 2024
0547c79
wip
SiarheiFedartsou May 25, 2024
272cbc9
wip
SiarheiFedartsou May 25, 2024
7553a3f
wip
SiarheiFedartsou May 25, 2024
01fd24f
wip
SiarheiFedartsou May 25, 2024
2666e96
wip
SiarheiFedartsou May 27, 2024
1d975e4
Merge branch 'master' into sf-std-variant
SiarheiFedartsou May 27, 2024
d0203d5
wip
SiarheiFedartsou May 27, 2024
1190303
wip
SiarheiFedartsou May 27, 2024
e7b1ee5
wip
SiarheiFedartsou May 27, 2024
4a1caae
wip
SiarheiFedartsou May 27, 2024
fb8a0d2
wip
SiarheiFedartsou May 27, 2024
6448423
wip
SiarheiFedartsou May 27, 2024
d292d52
wip
SiarheiFedartsou May 27, 2024
fd146f3
wip
SiarheiFedartsou May 27, 2024
1550c19
wip
SiarheiFedartsou May 27, 2024
23dc69c
wip
SiarheiFedartsou May 27, 2024
b9178c8
wip
SiarheiFedartsou May 27, 2024
d139d55
wip
SiarheiFedartsou May 27, 2024
47034c0
wip
SiarheiFedartsou May 27, 2024
ff27181
wip
SiarheiFedartsou May 27, 2024
fc16f49
wip
SiarheiFedartsou May 27, 2024
2815988
wip
SiarheiFedartsou May 27, 2024
381dd59
wip
SiarheiFedartsou May 27, 2024
cea393a
wip
SiarheiFedartsou May 27, 2024
164f7ea
wip
SiarheiFedartsou May 27, 2024
dea156d
wip
SiarheiFedartsou May 27, 2024
08a0724
wip
SiarheiFedartsou May 27, 2024
383b584
wip
SiarheiFedartsou May 27, 2024
7f747b7
wip
SiarheiFedartsou May 27, 2024
8ab618c
wip
SiarheiFedartsou May 27, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions .github/workflows/osrm-backend.yml
Original file line number Diff line number Diff line change
Expand Up @@ -275,15 +275,6 @@ jobs:
CXXCOMPILER: g++-12
CXXFLAGS: '-Wno-array-bounds -Wno-uninitialized'

- name: gcc-11-release
Copy link
Member Author

@SiarheiFedartsou SiarheiFedartsou May 27, 2024

Choose a reason for hiding this comment

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

GCC is much more capricious when it comes to std::variant. While with newer GCC versions I was able to make it working, but with GCC 11 it seems it doesn't like constructions like this:

struct Array;

using Value = std::variant<Array, ...>;

struct Array {
   std::vector<Value> values;
}

Workaround would be to use smth like boost::recursive_wrapper(event though there is no obvious need in it: compiler here has everything to calculate size of Value), but it would make code more complicated, so I chose the easier way: just drop GCC 11 job in favor of GCC 14 I recently added in another PR :) It seems GCC is getting better here: newer versions still had some issues I had no on Clang, but it was at least easy to fix.

Let me please know if there are any objections here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd be ok to drop GCC11. I think in the past™️ we had aimed to track the latest Debian release for minimum compilers. No idea which one that is tho. 😉

If you want to go with a recursive wrapper, here's a write up of mine from also some time ago on how to implement such a wrapper with like five lines of code in standard C++.

continue-on-error: false
node: 20
runs-on: ubuntu-22.04
BUILD_TOOLS: ON
BUILD_TYPE: Release
CCOMPILER: gcc-11
CXXCOMPILER: g++-11

- name: conan-linux-release-node
build_node_package: true
continue-on-error: false
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
- NodeJS:
- CHANGED: Use node-api instead of NAN. [#6452](https://github.com/Project-OSRM/osrm-backend/pull/6452)
- Misc:
- CHANGED: Use std::variant instead of mapbox::util::variant. [#6903](https://github.com/Project-OSRM/osrm-backend/pull/6903)
- CHANGED: Bump rapidjson to version f9d53419e912910fd8fa57d5705fa41425428c35 [#6906](https://github.com/Project-OSRM/osrm-backend/pull/6906)
- CHANGED: Bump mapbox/variant to version 1.2.0 [#6898](https://github.com/Project-OSRM/osrm-backend/pull/6898)
- CHANGED: Avoid copy of std::function-based callback in path unpacking [#6895](https://github.com/Project-OSRM/osrm-backend/pull/6895)
Expand Down
3 changes: 0 additions & 3 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,6 @@ endif()
include_directories(BEFORE ${CMAKE_CURRENT_BINARY_DIR}/include/)
include_directories(BEFORE ${CMAKE_CURRENT_SOURCE_DIR}/include/)
include_directories(SYSTEM ${CMAKE_CURRENT_SOURCE_DIR}/third_party/sol2-3.3.0/include)
include_directories(SYSTEM ${CMAKE_CURRENT_SOURCE_DIR}/third_party/variant/include)

set(BOOST_COMPONENTS date_time chrono filesystem iostreams program_options regex system thread unit_test_framework)

Expand Down Expand Up @@ -607,7 +606,6 @@ if (BUILD_ROUTED)
set_property(TARGET osrm-routed PROPERTY INSTALL_RPATH_USE_LINK_PATH TRUE)
endif()

file(GLOB VariantGlob third_party/variant/include/mapbox/*.hpp)
file(GLOB FlatbuffersGlob third_party/flatbuffers/include/flatbuffers/*.h)
file(GLOB LibraryGlob include/osrm/*.hpp)
file(GLOB ParametersGlob include/engine/api/*_parameters.hpp)
Expand All @@ -627,7 +625,6 @@ install(FILES ${ContractorHeader} DESTINATION include/osrm/contractor)
install(FILES ${LibraryGlob} DESTINATION include/osrm)
install(FILES ${ParametersGlob} DESTINATION include/osrm/engine/api)
install(FILES ${ApiHeader} DESTINATION include/osrm/engine/api)
install(FILES ${VariantGlob} DESTINATION include/mapbox)
install(FILES ${FlatbuffersGlob} DESTINATION include/flatbuffers)
install(TARGETS osrm-extract DESTINATION bin)
install(TARGETS osrm-partition DESTINATION bin)
Expand Down
14 changes: 7 additions & 7 deletions example/example.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,15 @@ int main(int argc, const char *argv[])
// Execute routing request, this does the heavy lifting
const auto status = osrm.Route(params, result);

auto &json_result = result.get<json::Object>();
auto &json_result = std::get<json::Object>(result);
if (status == Status::Ok)
{
auto &routes = json_result.values["routes"].get<json::Array>();
auto &routes = std::get<json::Array>(json_result.values["routes"]);

// Let's just use the first route
auto &route = routes.values.at(0).get<json::Object>();
const auto distance = route.values["distance"].get<json::Number>().value;
const auto duration = route.values["duration"].get<json::Number>().value;
auto &route = std::get<json::Object>(routes.values.at(0));
const auto distance = std::get<json::Number>(route.values["distance"]).value;
const auto duration = std::get<json::Number>(route.values["duration"]).value;

// Warn users if extract does not contain the default coordinates from above
if (distance == 0 || duration == 0)
Expand All @@ -80,8 +80,8 @@ int main(int argc, const char *argv[])
}
else if (status == Status::Error)
{
const auto code = json_result.values["code"].get<json::String>().value;
const auto message = json_result.values["message"].get<json::String>().value;
const auto code = std::get<json::String>(json_result.values["code"]).value;
const auto message = std::get<json::String>(json_result.values["message"]).value;

std::cout << "Code: " << code << "\n";
std::cout << "Message: " << code << "\n";
Expand Down
5 changes: 2 additions & 3 deletions include/engine/api/base_result.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,15 @@
#define ENGINE_API_BASE_RESULT_HPP

#include <flatbuffers/flatbuffers.h>
#include <mapbox/variant.hpp>
#include <variant>

#include <string>

#include "util/json_container.hpp"

namespace osrm::engine::api
{
using ResultT =
mapbox::util::variant<util::json::Object, std::string, flatbuffers::FlatBufferBuilder>;
using ResultT = std::variant<util::json::Object, std::string, flatbuffers::FlatBufferBuilder>;
} // namespace osrm::engine::api

#endif
4 changes: 2 additions & 2 deletions include/engine/api/json_factory.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ inline bool hasValidLanes(const guidance::IntermediateIntersection &intersection
return intersection.lanes.lanes_in_turn > 0;
}

util::json::Array coordinateToLonLat(const util::Coordinate &coordinate);
util::json::Value coordinateToLonLat(const util::Coordinate &coordinate);

/**
* Ensures that a bearing value is a whole number, and clamped to the range 0-359
Expand Down Expand Up @@ -79,7 +79,7 @@ util::json::Object makeGeoJSONGeometry(ForwardIter begin, ForwardIter end)
coordinates.values.push_back(location);
coordinates.values.push_back(location);
}
geojson.values["coordinates"] = std::move(coordinates);
geojson.values["coordinates"] = util::json::Value{std::move(coordinates)};

return geojson;
}
Expand Down
6 changes: 3 additions & 3 deletions include/engine/api/match_api.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,14 @@ class MatchAPI final : public RouteAPI
osrm::engine::api::ResultT &response) const
{
BOOST_ASSERT(sub_matchings.size() == sub_routes.size());
if (response.is<flatbuffers::FlatBufferBuilder>())
if (std::holds_alternative<flatbuffers::FlatBufferBuilder>(response))
{
auto &fb_result = response.get<flatbuffers::FlatBufferBuilder>();
auto &fb_result = std::get<flatbuffers::FlatBufferBuilder>(response);
MakeResponse(sub_matchings, sub_routes, fb_result);
}
else
{
auto &json_result = response.get<util::json::Object>();
auto &json_result = std::get<util::json::Object>(response);
MakeResponse(sub_matchings, sub_routes, json_result);
}
}
Expand Down
6 changes: 3 additions & 3 deletions include/engine/api/nearest_api.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,14 @@ class NearestAPI final : public BaseAPI
BOOST_ASSERT(phantom_nodes.size() == 1);
BOOST_ASSERT(parameters.coordinates.size() == 1);

if (response.is<flatbuffers::FlatBufferBuilder>())
if (std::holds_alternative<flatbuffers::FlatBufferBuilder>(response))
{
auto &fb_result = response.get<flatbuffers::FlatBufferBuilder>();
auto &fb_result = std::get<flatbuffers::FlatBufferBuilder>(response);
MakeResponse(phantom_nodes, fb_result);
}
else
{
auto &json_result = response.get<util::json::Object>();
auto &json_result = std::get<util::json::Object>(response);
MakeResponse(phantom_nodes, json_result);
}
}
Expand Down
19 changes: 9 additions & 10 deletions include/engine/api/route_api.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,14 @@ class RouteAPI : public BaseAPI
{
BOOST_ASSERT(!raw_routes.routes.empty());

if (response.is<flatbuffers::FlatBufferBuilder>())
if (std::holds_alternative<flatbuffers::FlatBufferBuilder>(response))
{
auto &fb_result = response.get<flatbuffers::FlatBufferBuilder>();
auto &fb_result = std::get<flatbuffers::FlatBufferBuilder>(response);
MakeResponse(raw_routes, waypoint_candidates, fb_result);
}
else
{
auto &json_result = response.get<util::json::Object>();
auto &json_result = std::get<util::json::Object>(response);
MakeResponse(raw_routes, waypoint_candidates, json_result);
}
}
Expand Down Expand Up @@ -158,8 +158,8 @@ class RouteAPI : public BaseAPI
}

template <typename ForwardIter>
mapbox::util::variant<flatbuffers::Offset<flatbuffers::String>,
flatbuffers::Offset<flatbuffers::Vector<const fbresult::Position *>>>
std::variant<flatbuffers::Offset<flatbuffers::String>,
flatbuffers::Offset<flatbuffers::Vector<const fbresult::Position *>>>
MakeGeometry(flatbuffers::FlatBufferBuilder &builder, ForwardIter begin, ForwardIter end) const
{
if (parameters.geometries == RouteParameters::GeometriesType::Polyline)
Expand Down Expand Up @@ -408,8 +408,8 @@ class RouteAPI : public BaseAPI

// Fill geometry
auto overview = MakeOverview(leg_geometries);
mapbox::util::variant<flatbuffers::Offset<flatbuffers::String>,
flatbuffers::Offset<flatbuffers::Vector<const fbresult::Position *>>>
std::variant<flatbuffers::Offset<flatbuffers::String>,
flatbuffers::Offset<flatbuffers::Vector<const fbresult::Position *>>>
geometry;
if (overview)
{
Expand All @@ -426,8 +426,7 @@ class RouteAPI : public BaseAPI
routeObject.add_legs(legs_vector);
if (overview)
{
mapbox::util::apply_visitor(GeometryVisitor<fbresult::RouteObjectBuilder>(routeObject),
geometry);
std::visit(GeometryVisitor<fbresult::RouteObjectBuilder>(routeObject), geometry);
}

return routeObject.Finish();
Expand Down Expand Up @@ -644,7 +643,7 @@ class RouteAPI : public BaseAPI
stepBuilder.add_rotary_pronunciation(rotary_pronunciation_string);
stepBuilder.add_intersections(intersections_vector);
stepBuilder.add_maneuver(maneuver_buffer);
mapbox::util::apply_visitor(GeometryVisitor<fbresult::StepBuilder>(stepBuilder), geometry);
std::visit(GeometryVisitor<fbresult::StepBuilder>(stepBuilder), geometry);
return stepBuilder.Finish();
};

Expand Down
32 changes: 18 additions & 14 deletions include/engine/api/table_api.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,14 @@ class TableAPI final : public BaseAPI
const std::vector<TableCellRef> &fallback_speed_cells,
osrm::engine::api::ResultT &response) const
{
if (response.is<flatbuffers::FlatBufferBuilder>())
if (std::holds_alternative<flatbuffers::FlatBufferBuilder>(response))
{
auto &fb_result = response.get<flatbuffers::FlatBufferBuilder>();
auto &fb_result = std::get<flatbuffers::FlatBufferBuilder>(response);
MakeResponse(tables, candidates, fallback_speed_cells, fb_result);
}
else
{
auto &json_result = response.get<util::json::Object>();
auto &json_result = std::get<util::json::Object>(response);
MakeResponse(tables, candidates, fallback_speed_cells, json_result);
}
}
Expand Down Expand Up @@ -377,7 +377,8 @@ class TableAPI final : public BaseAPI
return util::json::Value(
util::json::Number(from_alias<double>(duration) / 10.));
});
json_table.values.push_back(std::move(json_row));

json_table.values.push_back(util::json::Value{json_row});
}
return json_table;
}
Expand Down Expand Up @@ -406,7 +407,7 @@ class TableAPI final : public BaseAPI
return util::json::Value(util::json::Number(
std::round(from_alias<double>(distance) * 10) / 10.));
});
json_table.values.push_back(std::move(json_row));
json_table.values.push_back(util::json::Value{json_row});
}
return json_table;
}
Expand All @@ -415,15 +416,18 @@ class TableAPI final : public BaseAPI
MakeEstimatesTable(const std::vector<TableCellRef> &fallback_speed_cells) const
{
util::json::Array json_table;
std::for_each(fallback_speed_cells.begin(),
fallback_speed_cells.end(),
[&](const auto &cell)
{
util::json::Array row;
row.values.push_back(util::json::Number(cell.row));
row.values.push_back(util::json::Number(cell.column));
json_table.values.push_back(std::move(row));
});
std::for_each(
fallback_speed_cells.begin(),
fallback_speed_cells.end(),
[&](const auto &cell)
{
util::json::Array row;
util::json::Value jCellRow{util::json::Number(static_cast<double>(cell.row))};
util::json::Value jCellColumn{util::json::Number(static_cast<double>(cell.column))};
row.values.push_back(jCellRow);
row.values.push_back(jCellColumn);
json_table.values.push_back(util::json::Value{row});
});
return json_table;
}

Expand Down
6 changes: 3 additions & 3 deletions include/engine/api/trip_api.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,14 @@ class TripAPI final : public RouteAPI
{
BOOST_ASSERT(sub_trips.size() == sub_routes.size());

if (response.is<flatbuffers::FlatBufferBuilder>())
if (std::holds_alternative<flatbuffers::FlatBufferBuilder>(response))
{
auto &fb_result = response.get<flatbuffers::FlatBufferBuilder>();
auto &fb_result = std::get<flatbuffers::FlatBufferBuilder>(response);
MakeResponse(sub_trips, sub_routes, candidates, fb_result);
}
else
{
auto &json_result = response.get<util::json::Object>();
auto &json_result = std::get<util::json::Object>(response);
MakeResponse(sub_trips, sub_routes, candidates, json_result);
}
}
Expand Down
2 changes: 1 addition & 1 deletion include/engine/plugins/plugin_base.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class BasePlugin
const std::string &message,
osrm::engine::api::ResultT &result) const
{
mapbox::util::apply_visitor(ErrorRenderer(code, message), result);
std::visit(ErrorRenderer(code, message), result);
return Status::Error;
}

Expand Down
2 changes: 1 addition & 1 deletion include/extractor/internal_extractor_edge.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
#include "util/typedefs.hpp"

#include <boost/assert.hpp>
#include <mapbox/variant.hpp>
#include <utility>
#include <variant>

namespace osrm::extractor
{
Expand Down
2 changes: 1 addition & 1 deletion include/extractor/maneuver_override.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
#include "util/std_hash.hpp"
#include "util/vector_view.hpp"

#include <mapbox/variant.hpp>
#include <variant>

#include <algorithm>

Expand Down
4 changes: 1 addition & 3 deletions include/extractor/restriction.hpp
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
#ifndef RESTRICTION_HPP
#define RESTRICTION_HPP

#include "turn_path.hpp"
#include "util/coordinate.hpp"
#include "util/opening_hours.hpp"
#include "util/typedefs.hpp"

#include "mapbox/variant.hpp"
#include "turn_path.hpp"
#include <limits>

namespace osrm::extractor
Expand Down
Loading
Loading