From 93de1f16b3905e742901d1a5768d30d3b49beb8a Mon Sep 17 00:00:00 2001 From: FILLAU Jean-Maxime Date: Fri, 19 May 2017 14:41:27 +0200 Subject: [PATCH] Change qi::lit for qi::symbols for the sides parameter parser. Refactor code : - Suppress StartSide Enum - Change Side Structure for Enum Signed-off-by: FILLAU Jean-Maxime --- .../contiguous_internalmem_datafacade.hpp | 8 ++-- include/engine/datafacade/datafacade_base.hpp | 2 +- include/engine/geospatial_query.hpp | 11 ++--- include/engine/plugins/plugin_base.hpp | 8 ++-- include/engine/side.hpp | 40 +------------------ include/nodejs/node_osrm_support.hpp | 24 +++++++++-- include/osrm/side.hpp | 38 ++++++++++++++++++ .../server/api/base_parameters_grammar.hpp | 23 +++++------ unit_tests/mocks/mock_datafacade.hpp | 5 +-- unit_tests/server/parameters_io.hpp | 2 +- unit_tests/server/parameters_parser.cpp | 27 ++++++------- 11 files changed, 98 insertions(+), 90 deletions(-) create mode 100644 include/osrm/side.hpp diff --git a/include/engine/datafacade/contiguous_internalmem_datafacade.hpp b/include/engine/datafacade/contiguous_internalmem_datafacade.hpp index 6c32ed1fecd..ba8ed6a9fb7 100644 --- a/include/engine/datafacade/contiguous_internalmem_datafacade.hpp +++ b/include/engine/datafacade/contiguous_internalmem_datafacade.hpp @@ -714,14 +714,14 @@ class ContiguousInternalMemoryDataFacadeBase : public BaseDataFacade input_coordinate, max_results, max_distance, bearing, bearing_range); } - std::pair NearestPhantomNodeWithAlternativeFromBigComponent( - const util::Coordinate input_coordinate, - const engine::SideValue side_value) const override final + std::pair + NearestPhantomNodeWithAlternativeFromBigComponent(const util::Coordinate input_coordinate, + const engine::Side side) const override final { BOOST_ASSERT(m_geospatial_query.get()); return m_geospatial_query->NearestPhantomNodeWithAlternativeFromBigComponent( - input_coordinate, side_value); + input_coordinate, side); } std::pair NearestPhantomNodeWithAlternativeFromBigComponent( diff --git a/include/engine/datafacade/datafacade_base.hpp b/include/engine/datafacade/datafacade_base.hpp index 48f9bbe9cd5..cd467478e44 100644 --- a/include/engine/datafacade/datafacade_base.hpp +++ b/include/engine/datafacade/datafacade_base.hpp @@ -120,7 +120,7 @@ class BaseDataFacade virtual std::pair NearestPhantomNodeWithAlternativeFromBigComponent(const util::Coordinate input_coordinate, - const engine::SideValue side_value) const = 0; + const engine::Side side) const = 0; virtual std::pair NearestPhantomNodeWithAlternativeFromBigComponent(const util::Coordinate input_coordinate, const double max_distance) const = 0; diff --git a/include/engine/geospatial_query.hpp b/include/engine/geospatial_query.hpp index e9e612f370e..fff633f7f90 100644 --- a/include/engine/geospatial_query.hpp +++ b/include/engine/geospatial_query.hpp @@ -212,19 +212,20 @@ template class GeospatialQuery // a second phantom node is return that is the nearest coordinate in a big component. std::pair NearestPhantomNodeWithAlternativeFromBigComponent(const util::Coordinate input_coordinate, - const engine::SideValue side_value) const + const engine::Side side) const { bool has_small_component = false; bool has_big_component = false; auto results = rtree.Nearest( input_coordinate, - [this, &side_value, &input_coordinate, &has_big_component, &has_small_component](const CandidateSegment &segment) { + [this, &side, &input_coordinate, &has_big_component, &has_small_component]( + const CandidateSegment &segment) { auto use_segment = (!has_small_component || (!has_big_component && !IsTinyComponent(segment))); auto use_directions = std::make_pair(use_segment, use_segment); bool isOnewaySegment = !(segment.data.forward_segment_id.enabled && segment.data.reverse_segment_id.enabled); - if (!isOnewaySegment && side_value != BOTH) + if (!isOnewaySegment && side != BOTH) { // Check the counter clockwise // @@ -241,8 +242,8 @@ template class GeospatialQuery // if drive left // input_coordinate_is_at_right = !input_coordinate_is_at_right - // We reverse goCountrySide if side_value is OPPOSITE - if (side_value == OPPOSITE) + // We reverse goCountrySide if side is OPPOSITE + if (side == OPPOSITE) input_coordinate_is_at_right = !input_coordinate_is_at_right; // Apply the side. diff --git a/include/engine/plugins/plugin_base.hpp b/include/engine/plugins/plugin_base.hpp index 969bd1a2893..4717a4733d5 100644 --- a/include/engine/plugins/plugin_base.hpp +++ b/include/engine/plugins/plugin_base.hpp @@ -231,11 +231,11 @@ class BasePlugin BOOST_ASSERT(parameters.IsValid()); for (const auto i : util::irange(0UL, parameters.coordinates.size())) { - SideValue sideValue = engine::SideValue::BOTH; + Side side = engine::Side::BOTH; // TODO init at SIDE for test - // SideValue sideValue = engine::SideValue::DEFAULT; + // SideValue side = engine::SideValue::DEFAULT; if (use_sides && parameters.sides[i]) - sideValue = parameters.sides[i]->side; + side = parameters.sides[i].get(); if (use_hints && parameters.hints[i] && parameters.hints[i]->IsValid(parameters.coordinates[i], facade)) @@ -277,7 +277,7 @@ class BasePlugin { phantom_node_pairs[i] = facade.NearestPhantomNodeWithAlternativeFromBigComponent( - parameters.coordinates[i], sideValue); + parameters.coordinates[i], side); } } diff --git a/include/engine/side.hpp b/include/engine/side.hpp index 6866bff85f2..b1cf8a6666f 100644 --- a/include/engine/side.hpp +++ b/include/engine/side.hpp @@ -28,56 +28,18 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #ifndef OSRM_ENGINE_SIDE_HPP #define OSRM_ENGINE_SIDE_HPP -#include - namespace osrm { namespace engine { -enum SideValue +enum Side { DEFAULT, OPPOSITE, BOTH }; - -struct Side -{ - SideValue side; - static SideValue fromString(const std::string &str) - { - if (str == "d") - return DEFAULT; - else if (str == "o") - return OPPOSITE; - else - return BOTH; - } - static std::string toString(const Side &side) - { - switch(side.side) - { - case(DEFAULT) : - return "0"; - case(OPPOSITE) : - return "d"; - case(BOTH) : - return "b"; - default : - //TODO I don't know what to do here. - return "b"; - } - } -}; - -inline bool operator==(const Side lhs, const Side rhs) -{ - return lhs.side == rhs.side; } -inline bool operator!=(const Side lhs, const Side rhs) { return !(lhs == rhs); } } -} - #endif diff --git a/include/nodejs/node_osrm_support.hpp b/include/nodejs/node_osrm_support.hpp index 2ece7d9347b..cf4c9103f4d 100644 --- a/include/nodejs/node_osrm_support.hpp +++ b/include/nodejs/node_osrm_support.hpp @@ -11,6 +11,7 @@ #include "osrm/nearest_parameters.hpp" #include "osrm/osrm.hpp" #include "osrm/route_parameters.hpp" +#include "osrm/side.hpp" #include "osrm/status.hpp" #include "osrm/storage_config.hpp" #include "osrm/table_parameters.hpp" @@ -296,7 +297,7 @@ inline bool argumentsToParameter(const Nan::FunctionCallbackInfo &arg Nan::ThrowError("Coordinates must be an array of (lon/lat) pairs"); return false; } - + if (obj->Has(Nan::New("sides").ToLocalChecked())) { v8::Local sides = obj->Get(Nan::New("sides").ToLocalChecked()); @@ -325,13 +326,28 @@ inline bool argumentsToParameter(const Nan::FunctionCallbackInfo &arg if (side->IsString()) { - if (side->ToString()->Length() == 0) + if (!side->ToString().IsOneByte()) { - Nan::ThrowError("Side cannot be an empty string"); + Nan::ThrowError("Side can only take 'd' 'b' or 'o' values"); return false; } - params->sides.push_back(Side{sides}); + v8::String::Utf8Value arg(side[0]->ToString()); + std::string d_string("d"); + Handle d = ; + + + if (arg == String::New(string('b'))) + params->sides.push_back(osrm::engine::BOTH); + else if (arg == String::New(string('d'))) + params->sides.push_back(osrm::engine::DEFAULT); + else if (arg == String::New(string('o')) || side->ToString()->Length() == 0) + params->sides.push_back(osrm::engine::OPPOSITE); + else + { + Nan::ThrowError("Side can only take 'd' 'b' or 'o' values"); + return false; + } } else if (side->IsNull()) { diff --git a/include/osrm/side.hpp b/include/osrm/side.hpp new file mode 100644 index 00000000000..eb4448cfbb6 --- /dev/null +++ b/include/osrm/side.hpp @@ -0,0 +1,38 @@ +/* + +Copyright (c) 2016, Project OSRM contributors +All rights reserved. + +Redistribution and use in source and binary forms, with or without modification, +are permitted provided that the following conditions are met: + +Redistributions of source code must retain the above copyright notice, this list +of conditions and the following disclaimer. +Redistributions in binary form must reproduce the above copyright notice, this +list of conditions and the following disclaimer in the documentation and/or +other materials provided with the distribution. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND +ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED +WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE +DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR +ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES +(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; +LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON +ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS +SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +*/ + +#ifndef OSRM_SIDE_HPP +#define OSRM_SIDE_HPP + +#include "engine/side.hpp" + +namespace osrm +{ +using engine::Side; +} + +#endif diff --git a/include/server/api/base_parameters_grammar.hpp b/include/server/api/base_parameters_grammar.hpp index 7741bf21b3b..7cb72a93d71 100644 --- a/include/server/api/base_parameters_grammar.hpp +++ b/include/server/api/base_parameters_grammar.hpp @@ -99,16 +99,6 @@ struct BaseParametersGrammar : boost::spirit::qi::grammar base_parameters.bearings.push_back(std::move(bearing)); }; - const auto add_side = [](engine::api::BaseParameters &base_parameters, - const boost::optional &side_string) { - boost::optional side; - if (side_string) - { - side = engine::Side{engine::Side::fromString(side_string.get())}; - } - base_parameters.sides.push_back(std::move(side)); - }; - polyline_chars = qi::char_("a-zA-Z0-9_.--[]{}@?|\\%~`^"); base64_char = qi::char_("a-zA-Z0-9--_="); unlimited_rule = qi::lit("unlimited")[qi::_val = std::numeric_limits::infinity()]; @@ -159,13 +149,16 @@ struct BaseParametersGrammar : boost::spirit::qi::grammar qi::lit("bearings=") > (-(qi::short_ > ',' > qi::short_))[ph::bind(add_bearing, qi::_r1, qi::_1)] % ';'; - sides_rule = qi::lit("sides=") > - (-qi::as_string[qi::char_("a-zA")])[ph::bind(add_side, qi::_r1, qi::_1)] % ';'; + side_type.add("d", engine::Side::DEFAULT)("o", engine::Side::OPPOSITE)("b", engine::Side::BOTH); + sides_rule = + qi::lit("sides=") > + (-side_type % ';')[ph::bind(&engine::api::BaseParameters::sides, qi::_r1) = qi::_1]; base_rule = radiuses_rule(qi::_r1) // | hints_rule(qi::_r1) // | bearings_rule(qi::_r1) // - | generate_hints_rule(qi::_r1) | sides_rule(qi::_r1); + | generate_hints_rule(qi::_r1) + | sides_rule(qi::_r1); } protected: @@ -176,9 +169,9 @@ struct BaseParametersGrammar : boost::spirit::qi::grammar qi::rule bearings_rule; qi::rule radiuses_rule; qi::rule hints_rule; - qi::rule sides_rule; qi::rule generate_hints_rule; + qi::rule sides_rule; qi::rule bearing_rule; qi::rule location_rule; @@ -188,6 +181,8 @@ struct BaseParametersGrammar : boost::spirit::qi::grammar qi::rule polyline_chars; qi::rule unlimited_rule; qi::real_parser double_; + + qi::symbols side_type; }; } } diff --git a/unit_tests/mocks/mock_datafacade.hpp b/unit_tests/mocks/mock_datafacade.hpp index 9b695ffe6f2..972b780421d 100644 --- a/unit_tests/mocks/mock_datafacade.hpp +++ b/unit_tests/mocks/mock_datafacade.hpp @@ -146,9 +146,8 @@ class MockBaseDataFacade : public engine::datafacade::BaseDataFacade } std::pair - NearestPhantomNodeWithAlternativeFromBigComponent( - const util::Coordinate /*input_coordinate*/, - const engine::SideValue /*side_value*/) const override + NearestPhantomNodeWithAlternativeFromBigComponent(const util::Coordinate /*input_coordinate*/, + const engine::Side /*side*/) const override { return {}; } diff --git a/unit_tests/server/parameters_io.hpp b/unit_tests/server/parameters_io.hpp index da706eb754e..e044cbe773d 100644 --- a/unit_tests/server/parameters_io.hpp +++ b/unit_tests/server/parameters_io.hpp @@ -57,7 +57,7 @@ inline std::ostream &operator<<(std::ostream &out, Bearing bearing) inline std::ostream &operator<<(std::ostream &out, Side side) { - out << Side::toString(side); + out << side; return out; } } diff --git a/unit_tests/server/parameters_parser.cpp b/unit_tests/server/parameters_parser.cpp index a4cc2069c4f..d8f8eab84ad 100644 --- a/unit_tests/server/parameters_parser.cpp +++ b/unit_tests/server/parameters_parser.cpp @@ -46,7 +46,7 @@ BOOST_AUTO_TEST_CASE(invalid_route_urls) BOOST_CHECK_EQUAL(testInvalidOptions("1,2;3,4?overview=false&radiuses=foo"), 32UL); BOOST_CHECK_EQUAL(testInvalidOptions("1,2;3,4?overview=false&sides=foo"), - 30UL); + 29UL); BOOST_CHECK_EQUAL(testInvalidOptions("1,2;3,4?overview=false&hints=foo"), 29UL); BOOST_CHECK_EQUAL(testInvalidOptions("1,2;3,4?overview=false&hints=;;; ;"), @@ -400,22 +400,19 @@ BOOST_AUTO_TEST_CASE(valid_route_urls) BOOST_CHECK_EQUAL(result_17->annotations, true); std::vector> sides_18 = { - boost::none, - engine::Side{engine::SideValue::DEFAULT}, - engine::Side{engine::SideValue::BOTH}, - engine::Side{engine::SideValue::OPPOSITE}, + boost::none, engine::Side::DEFAULT, engine::Side::BOTH, engine::Side::OPPOSITE, }; RouteParameters reference_18{false, - false, - false, - RouteParameters::GeometriesType::Polyline, - RouteParameters::OverviewType::Simplified, - boost::optional{}, - coords_3, - std::vector>{}, - std::vector>{}, - std::vector>{}, - sides_18}; + false, + false, + RouteParameters::GeometriesType::Polyline, + RouteParameters::OverviewType::Simplified, + boost::optional{}, + coords_3, + std::vector>{}, + std::vector>{}, + std::vector>{}, + sides_18}; auto result_18 = parseParameters("1,2;3,4;5,6;7,8?steps=false&sides=;d;b;o"); BOOST_CHECK(result_18);