From 018a9bc80449d85c0e40fa5215377d8a03264c88 Mon Sep 17 00:00:00 2001 From: Tom Peoples Date: Thu, 19 Sep 2019 17:30:21 +1000 Subject: [PATCH 1/2] Removed un-needed calls to std::move These calls were throwing a pessimistic move error and stopping compilation. --- .travis.yml | 8 ++++++++ CHANGELOG.md | 1 + include/updater/csv_file_parser.hpp | 2 +- src/server/api/parameters_parser.cpp | 3 +-- 4 files changed, 11 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index 91e8ce14c..69530f94c 100644 --- a/.travis.yml +++ b/.travis.yml @@ -121,6 +121,14 @@ matrix: packages: ['libstdc++-4.9-dev'] env: CLANG_VERSION='5.0.0' BUILD_TYPE='Release' ENABLE_MASON=ON RUN_CLANG_FORMAT=ON ENABLE_LTO=ON + - os: linux + compiler: "gcc-9-release" + addons: &gcc9 + apt: + sources: ['ubuntu-toolchain-r-test'] + packages: ['g++-9', 'libbz2-dev', 'libxml2-dev', 'libzip-dev', 'liblua5.2-dev', 'libtbb-dev', 'libboost-all-dev'] + env: CCOMPILER='gcc-9' CXXCOMPILER='g++-9' BUILD_TYPE='Release' CXXFLAGS='-Wno-cast-function-type' + - os: linux compiler: "gcc-8-release" addons: &gcc8 diff --git a/CHANGELOG.md b/CHANGELOG.md index 85d75a301..c9ca7a8c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ - Changes from 5.21.0 - Build: - ADDED: optionally build Node `lts` and `latest` bindings [#5347](https://github.com/Project-OSRM/osrm-backend/pull/5347) + - FIXED: pessimistic calls to std::move [#5560](https://github.com/Project-OSRM/osrm-backend/pull/5561) - Features: - ADDED: new waypoints parameter to the `route` plugin, enabling silent waypoints [#5345](https://github.com/Project-OSRM/osrm-backend/pull/5345) - ADDED: data timestamp information in the response (saved in new file `.osrm.timestamp`). [#5115](https://github.com/Project-OSRM/osrm-backend/issues/5115) diff --git a/include/updater/csv_file_parser.hpp b/include/updater/csv_file_parser.hpp index f3221672b..e0c7fabc0 100644 --- a/include/updater/csv_file_parser.hpp +++ b/include/updater/csv_file_parser.hpp @@ -122,7 +122,7 @@ template struct CSVFilesParser util::Log() << "Loaded " << filename << " with " << result.size() << "values"; - return std::move(result); + return result; } catch (const boost::exception &e) { diff --git a/src/server/api/parameters_parser.cpp b/src/server/api/parameters_parser.cpp index a2c6767bd..db935fb5b 100644 --- a/src/server/api/parameters_parser.cpp +++ b/src/server/api/parameters_parser.cpp @@ -45,9 +45,8 @@ boost::optional parseParameters(std::string::iterator &iter, const auto ok = boost::spirit::qi::parse(iter, end, grammar(boost::phoenix::ref(parameters))); - // return move(a.b) is needed to move b out of a and then return the rvalue by implicit move if (ok && iter == end) - return std::move(parameters); + return parameters; } catch (const qi::expectation_failure &failure) { From 28895373fb6a56cd607cc62f608878c64ee0a54d Mon Sep 17 00:00:00 2001 From: Tom Peoples Date: Thu, 26 Sep 2019 17:36:31 +1000 Subject: [PATCH 2/2] Fixed flatbufferbuiler copy issues. Compiling under gcc9.1 we get copy issues. It appears we shouldn't pass builder classes by value, only ref. --- include/engine/api/base_api.hpp | 29 +++++++++++++++-------------- include/engine/api/match_api.hpp | 20 ++++++++++---------- include/engine/api/nearest_api.hpp | 7 +++---- include/engine/api/route_api.hpp | 20 ++++++++++---------- include/engine/api/table_api.hpp | 4 ++-- include/engine/api/trip_api.hpp | 12 ++++++------ 6 files changed, 46 insertions(+), 46 deletions(-) diff --git a/include/engine/api/base_api.hpp b/include/engine/api/base_api.hpp index b781bf245..0b927b00c 100644 --- a/include/engine/api/base_api.hpp +++ b/include/engine/api/base_api.hpp @@ -12,6 +12,7 @@ #include #include +#include #include namespace osrm @@ -73,7 +74,7 @@ class BaseAPI } flatbuffers::Offset>> - MakeWaypoints(flatbuffers::FlatBufferBuilder &builder, + MakeWaypoints(flatbuffers::FlatBufferBuilder *builder, const std::vector &segment_end_coordinates) const { BOOST_ASSERT(parameters.coordinates.size() > 0); @@ -82,43 +83,43 @@ class BaseAPI std::vector> waypoints; waypoints.resize(parameters.coordinates.size()); waypoints[0] = - MakeWaypoint(builder, segment_end_coordinates.front().source_phantom).Finish(); + MakeWaypoint(builder, segment_end_coordinates.front().source_phantom)->Finish(); std::transform(segment_end_coordinates.begin(), segment_end_coordinates.end(), std::next(waypoints.begin()), - [this, &builder](const PhantomNodes &phantom_pair) { - return MakeWaypoint(builder, phantom_pair.target_phantom).Finish(); + [this, builder](const PhantomNodes &phantom_pair) { + return MakeWaypoint(builder, phantom_pair.target_phantom)->Finish(); }); - return builder.CreateVector(waypoints); + return builder->CreateVector(waypoints); } // FIXME: gcc 4.9 does not like MakeWaypoints to be protected // protected: - fbresult::WaypointBuilder MakeWaypoint(flatbuffers::FlatBufferBuilder &builder, - const PhantomNode &phantom) const + std::unique_ptr MakeWaypoint(flatbuffers::FlatBufferBuilder *builder, + const PhantomNode &phantom) const { auto location = fbresult::Position(static_cast(util::toFloating(phantom.location.lon)), static_cast(util::toFloating(phantom.location.lat))); - auto name_string = builder.CreateString( + auto name_string = builder->CreateString( facade.GetNameForID(facade.GetNameIndex(phantom.forward_segment_id.id)).to_string()); boost::optional> hint_string = boost::none; if (parameters.generate_hints) { - hint_string = builder.CreateString(Hint{phantom, facade.GetCheckSum()}.ToBase64()); + hint_string = builder->CreateString(Hint{phantom, facade.GetCheckSum()}.ToBase64()); } - fbresult::WaypointBuilder waypoint(builder); - waypoint.add_location(&location); - waypoint.add_distance(util::coordinate_calculation::fccApproximateDistance( + auto waypoint = std::make_unique(*builder); + waypoint->add_location(&location); + waypoint->add_distance(util::coordinate_calculation::fccApproximateDistance( phantom.location, phantom.input_location)); - waypoint.add_name(name_string); + waypoint->add_name(name_string); if (hint_string) { - waypoint.add_hint(*hint_string); + waypoint->add_hint(*hint_string); } return waypoint; } diff --git a/include/engine/api/match_api.hpp b/include/engine/api/match_api.hpp index d005c3a32..3aa9ddb78 100644 --- a/include/engine/api/match_api.hpp +++ b/include/engine/api/match_api.hpp @@ -62,10 +62,10 @@ class MatchAPI final : public RouteAPI if (data_version_string) { - response.add_data_version(*data_version_string); + response->add_data_version(*data_version_string); } - fb_result.Finish(response.Finish()); + fb_result.Finish(response->Finish()); } void MakeResponse(const std::vector &sub_matchings, const std::vector &sub_routes, @@ -141,29 +141,29 @@ class MatchAPI final : public RouteAPI } const auto &phantom = sub_matchings[matching_index.sub_matching_index].nodes[matching_index.point_index]; - auto waypoint = BaseAPI::MakeWaypoint(fb_result, phantom); - waypoint.add_matchings_index(matching_index.sub_matching_index); - waypoint.add_alternatives_count(sub_matchings[matching_index.sub_matching_index] - .alternatives_count[matching_index.point_index]); + auto waypoint = BaseAPI::MakeWaypoint(&fb_result, phantom); + waypoint->add_matchings_index(matching_index.sub_matching_index); + waypoint->add_alternatives_count(sub_matchings[matching_index.sub_matching_index] + .alternatives_count[matching_index.point_index]); // waypoint indices need to be adjusted if route legs were collapsed // waypoint parameter assumes there is only one match object if (!parameters.waypoints.empty()) { if (tidy_result.was_waypoint[trace_index]) { - waypoint.add_waypoint_index(was_waypoint_idx); + waypoint->add_waypoint_index(was_waypoint_idx); was_waypoint_idx++; } else { - waypoint.add_waypoint_index(0); + waypoint->add_waypoint_index(0); } } else { - waypoint.add_waypoint_index(matching_index.point_index); + waypoint->add_waypoint_index(matching_index.point_index); } - waypoints.push_back(waypoint.Finish()); + waypoints.push_back(waypoint->Finish()); } return fb_result.CreateVector(waypoints); diff --git a/include/engine/api/nearest_api.hpp b/include/engine/api/nearest_api.hpp index 6e0788ef4..4cc01e41f 100644 --- a/include/engine/api/nearest_api.hpp +++ b/include/engine/api/nearest_api.hpp @@ -71,10 +71,9 @@ class NearestAPI final : public BaseAPI auto node_values = MakeNodes(phantom_node); fbresult::Uint64Pair nodes{node_values.first, node_values.second}; - auto waypoint = MakeWaypoint(fb_result, phantom_node); - waypoint.add_nodes(&nodes); - - return waypoint.Finish(); + auto waypoint = MakeWaypoint(&fb_result, phantom_node); + waypoint->add_nodes(&nodes); + return waypoint->Finish(); }); waypoints_vector = fb_result.CreateVector(waypoints); diff --git a/include/engine/api/route_api.hpp b/include/engine/api/route_api.hpp index 8adac88ef..969c70121 100644 --- a/include/engine/api/route_api.hpp +++ b/include/engine/api/route_api.hpp @@ -81,14 +81,14 @@ class RouteAPI : public BaseAPI auto response = MakeFBResponse(raw_routes, fb_result, [this, &all_start_end_points, &fb_result]() { - return BaseAPI::MakeWaypoints(fb_result, all_start_end_points); + return BaseAPI::MakeWaypoints(&fb_result, all_start_end_points); }); if (data_version_string) { - response.add_data_version(*data_version_string); + response->add_data_version(*data_version_string); } - fb_result.Finish(response.Finish()); + fb_result.Finish(response->Finish()); } void @@ -125,9 +125,10 @@ class RouteAPI : public BaseAPI protected: template - fbresult::FBResultBuilder MakeFBResponse(const InternalManyRoutesResult &raw_routes, - flatbuffers::FlatBufferBuilder &fb_result, - GetWptsFn getWaypoints) const + std::unique_ptr + MakeFBResponse(const InternalManyRoutesResult &raw_routes, + flatbuffers::FlatBufferBuilder &fb_result, + GetWptsFn getWaypoints) const { std::vector> routes; @@ -151,9 +152,9 @@ class RouteAPI : public BaseAPI waypoints_vector = getWaypoints(); } - fbresult::FBResultBuilder response(fb_result); - response.add_routes(routes_vector); - response.add_waypoints(waypoints_vector); + auto response = std::make_unique(fb_result); + response->add_routes(routes_vector); + response->add_waypoints(waypoints_vector); return response; } @@ -656,7 +657,6 @@ class RouteAPI : public BaseAPI step.intersections.end(), intersections.begin(), [&fb_result, this](const guidance::IntermediateIntersection &intersection) { - std::vector> lanes; if (json::detail::hasValidLanes(intersection)) { diff --git a/include/engine/api/table_api.hpp b/include/engine/api/table_api.hpp index 4fe21b665..c459a5ae1 100644 --- a/include/engine/api/table_api.hpp +++ b/include/engine/api/table_api.hpp @@ -236,7 +236,7 @@ class TableAPI final : public BaseAPI boost::range::transform( phantoms, std::back_inserter(waypoints), [this, &builder](const PhantomNode &phantom) { - return BaseAPI::MakeWaypoint(builder, phantom).Finish(); + return BaseAPI::MakeWaypoint(&builder, phantom)->Finish(); }); return builder.CreateVector(waypoints); } @@ -252,7 +252,7 @@ class TableAPI final : public BaseAPI std::back_inserter(waypoints), [this, &builder, phantoms](const std::size_t idx) { BOOST_ASSERT(idx < phantoms.size()); - return BaseAPI::MakeWaypoint(builder, phantoms[idx]).Finish(); + return BaseAPI::MakeWaypoint(&builder, phantoms[idx])->Finish(); }); return builder.CreateVector(waypoints); } diff --git a/include/engine/api/trip_api.hpp b/include/engine/api/trip_api.hpp index a38275f54..f93701373 100644 --- a/include/engine/api/trip_api.hpp +++ b/include/engine/api/trip_api.hpp @@ -61,9 +61,9 @@ class TripAPI final : public RouteAPI if (data_version_string) { - response.add_data_version(*data_version_string); + response->add_data_version(*data_version_string); } - fb_result.Finish(response.Finish()); + fb_result.Finish(response->Finish()); } void MakeResponse(const std::vector> &sub_trips, const std::vector &sub_routes, @@ -127,10 +127,10 @@ class TripAPI final : public RouteAPI auto trip_index = input_idx_to_trip_idx[input_index]; BOOST_ASSERT(!trip_index.NotUsed()); - auto waypoint = BaseAPI::MakeWaypoint(fb_result, phantoms[input_index]); - waypoint.add_waypoint_index(trip_index.point_index); - waypoint.add_trips_index(trip_index.sub_trip_index); - waypoints.push_back(waypoint.Finish()); + auto waypoint = BaseAPI::MakeWaypoint(&fb_result, phantoms[input_index]); + waypoint->add_waypoint_index(trip_index.point_index); + waypoint->add_trips_index(trip_index.sub_trip_index); + waypoints.push_back(waypoint->Finish()); } return fb_result.CreateVector(waypoints);