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 <[email protected]>
Project-OSRM · May 19, 2017 · 93de1f1 · 93de1f1
1 parent f8d663b
commit 93de1f1
Show file tree

Hide file tree

Showing 11 changed files with 98 additions and 90 deletions.
diff --git 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<PhantomNode, PhantomNode> NearestPhantomNodeWithAlternativeFromBigComponent(
-        const util::Coordinate input_coordinate,
-        const engine::SideValue side_value) const override final
+    std::pair<PhantomNode, PhantomNode>
+    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<PhantomNode, PhantomNode> NearestPhantomNodeWithAlternativeFromBigComponent(

diff --git a/include/engine/datafacade/datafacade_base.hpp b/include/engine/datafacade/datafacade_base.hpp
@@ -120,7 +120,7 @@ class BaseDataFacade
 
     virtual std::pair<PhantomNode, PhantomNode>
     NearestPhantomNodeWithAlternativeFromBigComponent(const util::Coordinate input_coordinate,
-                                                      const engine::SideValue side_value) const = 0;
+                                                      const engine::Side side) const = 0;
     virtual std::pair<PhantomNode, PhantomNode>
     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
@@ -212,19 +212,20 @@ template <typename RTreeT, typename DataFacadeT> class GeospatialQuery
     // a second phantom node is return that is the nearest coordinate in a big component.
     std::pair<PhantomNode, PhantomNode>
     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 <typename RTreeT, typename DataFacadeT> 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
@@ -231,11 +231,11 @@ class BasePlugin
         BOOST_ASSERT(parameters.IsValid());
         for (const auto i : util::irange<std::size_t>(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
@@ -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 <string>
-
 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
@@ -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<v8::Value> &arg
         Nan::ThrowError("Coordinates must be an array of (lon/lat) pairs");
         return false;
     }
- 
+
     if (obj->Has(Nan::New("sides").ToLocalChecked()))
     {
         v8::Local<v8::Value> sides = obj->Get(Nan::New("sides").ToLocalChecked());
@@ -325,13 +326,28 @@ inline bool argumentsToParameter(const Nan::FunctionCallbackInfo<v8::Value> &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<Value> 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
@@ -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
@@ -99,16 +99,6 @@ struct BaseParametersGrammar : boost::spirit::qi::grammar<Iterator, Signature>
                 base_parameters.bearings.push_back(std::move(bearing));
             };
 
-        const auto add_side = [](engine::api::BaseParameters &base_parameters,
-                                 const boost::optional<std::string> &side_string) {
-            boost::optional<engine::Side> 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<double>::infinity()];
@@ -159,13 +149,16 @@ struct BaseParametersGrammar : boost::spirit::qi::grammar<Iterator, Signature>
             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<Iterator, Signature>
     qi::rule<Iterator, Signature> bearings_rule;
     qi::rule<Iterator, Signature> radiuses_rule;
     qi::rule<Iterator, Signature> hints_rule;
-    qi::rule<Iterator, Signature> sides_rule;
 
     qi::rule<Iterator, Signature> generate_hints_rule;
+    qi::rule<Iterator, Signature> sides_rule;
 
     qi::rule<Iterator, osrm::engine::Bearing()> bearing_rule;
     qi::rule<Iterator, osrm::util::Coordinate()> location_rule;
@@ -188,6 +181,8 @@ struct BaseParametersGrammar : boost::spirit::qi::grammar<Iterator, Signature>
     qi::rule<Iterator, std::string()> polyline_chars;
     qi::rule<Iterator, double()> unlimited_rule;
     qi::real_parser<double, json_policy> double_;
+
+    qi::symbols<char, engine::Side> side_type;
 };
 }
 }

diff --git 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<engine::PhantomNode, engine::PhantomNode>
-    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
@@ -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
@@ -46,7 +46,7 @@ BOOST_AUTO_TEST_CASE(invalid_route_urls)
     BOOST_CHECK_EQUAL(testInvalidOptions<RouteParameters>("1,2;3,4?overview=false&radiuses=foo"),
                       32UL);
     BOOST_CHECK_EQUAL(testInvalidOptions<RouteParameters>("1,2;3,4?overview=false&sides=foo"),
-                      30UL);
+                      29UL);
     BOOST_CHECK_EQUAL(testInvalidOptions<RouteParameters>("1,2;3,4?overview=false&hints=foo"),
                       29UL);
     BOOST_CHECK_EQUAL(testInvalidOptions<RouteParameters>("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<boost::optional<engine::Side>> 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<bool>{},
-                                coords_3,
-                                std::vector<boost::optional<engine::Hint>>{},
-                                std::vector<boost::optional<double>>{},
-                                std::vector<boost::optional<engine::Bearing>>{},
-                                sides_18};
+                                 false,
+                                 false,
+                                 RouteParameters::GeometriesType::Polyline,
+                                 RouteParameters::OverviewType::Simplified,
+                                 boost::optional<bool>{},
+                                 coords_3,
+                                 std::vector<boost::optional<engine::Hint>>{},
+                                 std::vector<boost::optional<double>>{},
+                                 std::vector<boost::optional<engine::Bearing>>{},
+                                 sides_18};
 
     auto result_18 = parseParameters<RouteParameters>("1,2;3,4;5,6;7,8?steps=false&sides=;d;b;o");
     BOOST_CHECK(result_18);