From 0e5f71408e1bbc496ca83e8796605b0df2985a58 Mon Sep 17 00:00:00 2001
From: Colin Rundel <rundel@gmail.com>
Date: Tue, 28 Aug 2012 17:14:02 -0400
Subject: [PATCH 1/5] Simplified path_parse implementation

path_parse and path_parse_from_string were redundant, replaced with overloaded path_parse function to achieve the same functionality.

Additional consistency cleanup in load_map.cpp.
---
 include/mapnik/parse_path.hpp              |  10 +-
 include/mapnik/path_expression_grammar.hpp |   4 +-
 src/load_map.cpp                           | 107 ++++++++-------------
 src/parse_path.cpp                         |  20 ++--
 4 files changed, 56 insertions(+), 85 deletions(-)

diff --git a/include/mapnik/parse_path.hpp b/include/mapnik/parse_path.hpp
index 828c0204f..90b065ac9 100644
--- a/include/mapnik/parse_path.hpp
+++ b/include/mapnik/parse_path.hpp
@@ -28,6 +28,7 @@
 #include <mapnik/attribute.hpp>
 #include <mapnik/feature.hpp>
 #include <mapnik/value.hpp>
+#include <mapnik/path_expression_grammar.hpp>
 
 // boost
 #include <boost/shared_ptr.hpp>
@@ -40,16 +41,11 @@
 
 namespace mapnik {
 
-typedef boost::variant<std::string, attribute> path_component;
-typedef std::vector<path_component> path_expression;
 typedef boost::shared_ptr<path_expression> path_expression_ptr;
-template <typename Iterator> struct path_expression_grammar;
 
 MAPNIK_DECL path_expression_ptr parse_path(std::string const & str);
-MAPNIK_DECL bool parse_path_from_string(path_expression_ptr const& path,
-                                        std::string const & str,
-                                        path_expression_grammar<std::string::const_iterator> const& g);
-
+MAPNIK_DECL path_expression_ptr parse_path(std::string const & str,
+                                           path_expression_grammar<std::string::const_iterator> const& g);
 
 template <typename T>
 struct path_processor
diff --git a/include/mapnik/path_expression_grammar.hpp b/include/mapnik/path_expression_grammar.hpp
index f60f33feb..58f8345a2 100644
--- a/include/mapnik/path_expression_grammar.hpp
+++ b/include/mapnik/path_expression_grammar.hpp
@@ -39,14 +39,16 @@
 
 namespace mapnik
 {
+	
 using namespace boost;
 namespace qi = boost::spirit::qi;
 namespace phoenix = boost::phoenix;
 namespace standard_wide =  boost::spirit::standard_wide;
 
 using standard_wide::space_type;
-using standard_wide::space;
+
 typedef boost::variant<std::string, attribute> path_component;
+typedef std::vector<path_component> path_expression;
 
 template <typename Iterator>
 struct path_expression_grammar : qi::grammar<Iterator, std::vector<path_component>(), space_type>
diff --git a/src/load_map.cpp b/src/load_map.cpp
index d8ec28825..f7efc5488 100644
--- a/src/load_map.cpp
+++ b/src/load_map.cpp
@@ -899,13 +899,7 @@ void map_parser::parse_point_symbolizer(rule & rule, xml_node const & sym)
             *file = ensure_relative_to_xml(file);
             std::string filename = *file;
             ensure_exists(filename);
-            path_expression_ptr expr(boost::make_shared<path_expression>());
-            if (!parse_path_from_string(expr, filename, sym.get_tree().path_expr_grammar))
-            {
-                throw mapnik::config_error("Failed to parse path_expression '" + filename + "'");
-            }
-
-            symbol.set_filename(expr);
+            symbol.set_filename( parse_path(filename, sym.get_tree().path_expr_grammar) );
 
             optional<std::string> image_transform_wkt = sym.get_opt_attr<std::string>("transform");
             if (image_transform_wkt)
@@ -928,14 +922,13 @@ void map_parser::parse_point_symbolizer(rule & rule, xml_node const & sym)
     }
 }
 
-
-void map_parser::parse_markers_symbolizer(rule & rule, xml_node const& node)
+void map_parser::parse_markers_symbolizer(rule & rule, xml_node const& sym)
 {
     try
     {
         std::string filename("");
-        optional<std::string> file = node.get_opt_attr<std::string>("file");
-        optional<std::string> base = node.get_opt_attr<std::string>("base");
+        optional<std::string> file = sym.get_opt_attr<std::string>("file");
+        optional<std::string> base = sym.get_opt_attr<std::string>("base");
 
         if (file && !file->empty())
         {
@@ -951,7 +944,7 @@ void map_parser::parse_markers_symbolizer(rule & rule, xml_node const& node)
             filename = ensure_relative_to_xml(file);
         }
 
-        optional<std::string> marker_type = node.get_opt_attr<std::string>("marker-type");
+        optional<std::string> marker_type = sym.get_opt_attr<std::string>("marker-type");
         if (marker_type)
         {
             // TODO - revisit whether to officially deprecate marker-type
@@ -971,68 +964,67 @@ void map_parser::parse_markers_symbolizer(rule & rule, xml_node const& node)
             }
         }
 
-        markers_symbolizer sym;
+        markers_symbolizer symbol;
 
         if (!filename.empty())
         {
             ensure_exists(filename);
-            path_expression_ptr expr(boost::make_shared<path_expression>());
-            if (!parse_path_from_string(expr, filename, node.get_tree().path_expr_grammar))
-            {
-                throw mapnik::config_error("Failed to parse path_expression '" + filename + "'");
-            }
-            sym.set_filename(expr);
+            symbol.set_filename( parse_path(filename, sym.get_tree().path_expr_grammar) );
         }
 
         // overall opacity to be applied to all paths
-        optional<float> opacity = node.get_opt_attr<float>("opacity");
-        if (opacity) sym.set_opacity(*opacity);
+        optional<float> opacity = sym.get_opt_attr<float>("opacity");
+        if (opacity) symbol.set_opacity(*opacity);
 
-        optional<float> fill_opacity = node.get_opt_attr<float>("fill-opacity");
-        if (fill_opacity) sym.set_fill_opacity(*fill_opacity);
+        optional<float> fill_opacity = sym.get_opt_attr<float>("fill-opacity");
+        if (fill_opacity) symbol.set_fill_opacity(*fill_opacity);
 
-        optional<std::string> image_transform_wkt = node.get_opt_attr<std::string>("transform");
+        optional<std::string> image_transform_wkt = sym.get_opt_attr<std::string>("transform");
         if (image_transform_wkt)
         {
             mapnik::transform_list_ptr tl = boost::make_shared<mapnik::transform_list>();
-            if (!mapnik::parse_transform(*tl, *image_transform_wkt, node.get_tree().transform_expr_grammar))
+            if (!mapnik::parse_transform(*tl, *image_transform_wkt, sym.get_tree().transform_expr_grammar))
             {
                 throw mapnik::config_error("Failed to parse transform: '" + *image_transform_wkt + "'");
             }
-            sym.set_image_transform(tl);
+            symbol.set_image_transform(tl);
         }
 
-        optional<color> c = node.get_opt_attr<color>("fill");
-        if (c) sym.set_fill(*c);
-        optional<double> spacing = node.get_opt_attr<double>("spacing");
-        if (spacing) sym.set_spacing(*spacing);
-        optional<double> max_error = node.get_opt_attr<double>("max-error");
-        if (max_error) sym.set_max_error(*max_error);
-        optional<boolean> allow_overlap = node.get_opt_attr<boolean>("allow-overlap");
-        optional<boolean> ignore_placement = node.get_opt_attr<boolean>("ignore-placement");
-        if (allow_overlap) sym.set_allow_overlap(*allow_overlap);
-        if (ignore_placement) sym.set_ignore_placement(*ignore_placement);
+        optional<color> c = sym.get_opt_attr<color>("fill");
+        if (c) symbol.set_fill(*c);
+        
+        optional<double> spacing = sym.get_opt_attr<double>("spacing");
+        if (spacing) symbol.set_spacing(*spacing);
+        
+        optional<double> max_error = sym.get_opt_attr<double>("max-error");
+        if (max_error) symbol.set_max_error(*max_error);
+        
+        optional<boolean> allow_overlap = sym.get_opt_attr<boolean>("allow-overlap");
+        if (allow_overlap) symbol.set_allow_overlap(*allow_overlap);
+        
+        optional<boolean> ignore_placement = sym.get_opt_attr<boolean>("ignore-placement");
+        if (ignore_placement) symbol.set_ignore_placement(*ignore_placement);
 
-        optional<expression_ptr> width = node.get_opt_attr<expression_ptr>("width");
-        if (width) sym.set_width(*width);
+        optional<expression_ptr> width = sym.get_opt_attr<expression_ptr>("width");
+        if (width) symbol.set_width(*width);
 
-        optional<expression_ptr> height = node.get_opt_attr<expression_ptr>("height");
-        if (height) sym.set_height(*height);
+        optional<expression_ptr> height = sym.get_opt_attr<expression_ptr>("height");
+        if (height) symbol.set_height(*height);
 
         stroke strk;
-        if (parse_stroke(strk,node))
+        if (parse_stroke(strk,sym))
         {
-            sym.set_stroke(strk);
+            symbol.set_stroke(strk);
         }
 
-        marker_placement_e placement = node.get_attr<marker_placement_e>("placement",sym.get_marker_placement());
-        sym.set_marker_placement(placement);
-        parse_symbolizer_base(sym, node);
-        rule.append(sym);
+        marker_placement_e placement = sym.get_attr<marker_placement_e>("placement",symbol.get_marker_placement());
+        symbol.set_marker_placement(placement);
+        parse_symbolizer_base(symbol, sym);
+        rule.append(symbol);
     }
     catch (config_error const& ex)
     {
-        ex.append_context(node);
+        ex.append_context(sym);
         throw;
     }
 }
@@ -1060,12 +1052,7 @@ void map_parser::parse_line_pattern_symbolizer(rule & rule, xml_node const & sym
 
         file = ensure_relative_to_xml(file);
         ensure_exists(file);
-        path_expression_ptr expr(boost::make_shared<path_expression>());
-        if (!parse_path_from_string(expr, file, sym.get_tree().path_expr_grammar))
-        {
-            throw mapnik::config_error("Failed to parse path_expression '" + file + "'");
-        }
-        line_pattern_symbolizer symbol(expr);
+        line_pattern_symbolizer symbol( parse_path(file, sym.get_tree().path_expr_grammar) );
 
         parse_symbolizer_base(symbol, sym);
         rule.append(symbol);
@@ -1102,12 +1089,7 @@ void map_parser::parse_polygon_pattern_symbolizer(rule & rule,
 
         file = ensure_relative_to_xml(file);
         ensure_exists(file);
-        path_expression_ptr expr(boost::make_shared<path_expression>());
-        if (!parse_path_from_string(expr, file, sym.get_tree().path_expr_grammar))
-        {
-            throw mapnik::config_error("Failed to parse path_expression '" + file + "'");
-        }
-        polygon_pattern_symbolizer symbol(expr);
+        polygon_pattern_symbolizer symbol( parse_path(file, sym.get_tree().path_expr_grammar) );
 
         // pattern alignment
         pattern_alignment_e p_alignment = sym.get_attr<pattern_alignment_e>("alignment",LOCAL_ALIGNMENT);
@@ -1254,12 +1236,7 @@ void map_parser::parse_shield_symbolizer(rule & rule, xml_node const& sym)
 
         file = ensure_relative_to_xml(file);
         ensure_exists(file);
-        path_expression_ptr expr(boost::make_shared<path_expression>());
-        if (!parse_path_from_string(expr, file, sym.get_tree().path_expr_grammar))
-        {
-            throw mapnik::config_error("Failed to parse path_expression '" + file + "'");
-        }
-        shield_symbol.set_filename(expr);
+        shield_symbol.set_filename( parse_path(file, sym.get_tree().path_expr_grammar) );
         parse_symbolizer_base(shield_symbol, sym);
         rule.append(shield_symbol);
     }
diff --git a/src/parse_path.cpp b/src/parse_path.cpp
index 9e564653a..7932710ff 100644
--- a/src/parse_path.cpp
+++ b/src/parse_path.cpp
@@ -29,12 +29,18 @@ namespace mapnik {
 
 path_expression_ptr parse_path(std::string const & str)
 {
-    path_expression_ptr path =  boost::make_shared<path_expression>();
     path_expression_grammar<std::string::const_iterator> g;
+    return parse_path(str,g);
+}
 
+path_expression_ptr parse_path(std::string const & str,
+                               path_expression_grammar<std::string::const_iterator> const& g)
+{
+    path_expression_ptr path =  boost::make_shared<path_expression>();
+    
     std::string::const_iterator itr = str.begin();
     std::string::const_iterator end = str.end();
-    bool r = qi::phrase_parse(itr, end, g, space, *path);
+    bool r = qi::phrase_parse(itr, end, g, boost::spirit::standard_wide::space, *path);
     if (r  && itr == end)
     {
         return path;
@@ -45,14 +51,4 @@ path_expression_ptr parse_path(std::string const & str)
     }
 }
 
-bool parse_path_from_string(path_expression_ptr const& path,
-                                           std::string const & str,
-                                           path_expression_grammar<std::string::const_iterator> const& g)
-{
-    std::string::const_iterator itr = str.begin();
-    std::string::const_iterator end = str.end();
-    bool r = qi::phrase_parse(itr, end, g, space, *path);
-    return (r && itr==end);
-}
-
 }

From 1d0c8171705fa947bc686397245907acec3122b1 Mon Sep 17 00:00:00 2001
From: Colin Rundel <rundel@gmail.com>
Date: Wed, 29 Aug 2012 12:06:29 -0400
Subject: [PATCH 2/5] Make path_expression_ptr have const contents, cleanup

---
 include/mapnik/parse_path.hpp |  2 +-
 include/mapnik/rule.hpp       | 29 -----------------------------
 src/parse_path.cpp            | 12 ++++++------
 3 files changed, 7 insertions(+), 36 deletions(-)

diff --git a/include/mapnik/parse_path.hpp b/include/mapnik/parse_path.hpp
index 90b065ac9..e89d9adfa 100644
--- a/include/mapnik/parse_path.hpp
+++ b/include/mapnik/parse_path.hpp
@@ -41,7 +41,7 @@
 
 namespace mapnik {
 
-typedef boost::shared_ptr<path_expression> path_expression_ptr;
+typedef boost::shared_ptr<const path_expression> path_expression_ptr;
 
 MAPNIK_DECL path_expression_ptr parse_path(std::string const & str);
 MAPNIK_DECL path_expression_ptr parse_path(std::string const & str,
diff --git a/include/mapnik/rule.hpp b/include/mapnik/rule.hpp
index 24451a2d5..9b7cbf0ad 100644
--- a/include/mapnik/rule.hpp
+++ b/include/mapnik/rule.hpp
@@ -135,27 +135,6 @@ private:
 
     struct deepcopy_symbolizer : public boost::static_visitor<>
     {
-
-        void operator () (markers_symbolizer & sym) const
-        {
-            copy_path_ptr(sym);
-        }
-
-        void operator () (point_symbolizer & sym) const
-        {
-            copy_path_ptr(sym);
-        }
-
-        void operator () (polygon_pattern_symbolizer & sym) const
-        {
-            copy_path_ptr(sym);
-        }
-
-        void operator () (line_pattern_symbolizer & sym) const
-        {
-            copy_path_ptr(sym);
-        }
-
         void operator () (raster_symbolizer & sym) const
         {
             raster_colorizer_ptr old_colorizer = sym.get_colorizer();
@@ -174,7 +153,6 @@ private:
 
         void operator () (shield_symbolizer & sym) const
         {
-            copy_path_ptr(sym);
             copy_text_ptr(sym);
         }
 
@@ -183,19 +161,12 @@ private:
             copy_height_ptr(sym);
         }
 
-
         template <typename T> void operator () (T &sym) const
         {
             boost::ignore_unused_variable_warning(sym);
         }
 
     private:
-        template <class T>
-        void copy_path_ptr(T & sym) const
-        {
-            std::string path = path_processor_type::to_string(*sym.get_filename());
-            sym.set_filename( parse_path(path) );
-        }
 
         template <class T>
         void copy_text_ptr(T & sym) const
diff --git a/src/parse_path.cpp b/src/parse_path.cpp
index 7932710ff..96e1a4687 100644
--- a/src/parse_path.cpp
+++ b/src/parse_path.cpp
@@ -27,27 +27,27 @@
 
 namespace mapnik {
 
-path_expression_ptr parse_path(std::string const & str)
+path_expression_ptr parse_path(std::string const& str)
 {
     path_expression_grammar<std::string::const_iterator> g;
     return parse_path(str,g);
 }
 
-path_expression_ptr parse_path(std::string const & str,
+path_expression_ptr parse_path(std::string const& str,
                                path_expression_grammar<std::string::const_iterator> const& g)
 {
-    path_expression_ptr path =  boost::make_shared<path_expression>();
+    path_expression path;  
     
     std::string::const_iterator itr = str.begin();
     std::string::const_iterator end = str.end();
-    bool r = qi::phrase_parse(itr, end, g, boost::spirit::standard_wide::space, *path);
+    bool r = qi::phrase_parse(itr, end, g, boost::spirit::standard_wide::space, path);
     if (r  && itr == end)
     {
-        return path;
+        return boost::make_shared<const path_expression>(path); //path;
     }
     else
     {
-        throw std::runtime_error("Failed to parse path expression");
+        throw std::runtime_error("Failed to parse path expression: \"" + str + "\"");
     }
 }
 

From 10001f1d4b7a2e595f2eb632e497f7182470093a Mon Sep 17 00:00:00 2001
From: Colin Rundel <rundel@gmail.com>
Date: Wed, 29 Aug 2012 12:59:52 -0400
Subject: [PATCH 3/5] Expression parsing simplification

Similar idea to path expression parsing cleanup
---
 include/mapnik/expression.hpp | 17 +++++----------
 src/expression.cpp            | 39 ++++++++++++++---------------------
 src/xml_tree.cpp              | 10 +--------
 3 files changed, 21 insertions(+), 45 deletions(-)

diff --git a/include/mapnik/expression.hpp b/include/mapnik/expression.hpp
index 8862aed76..7a8751c02 100644
--- a/include/mapnik/expression.hpp
+++ b/include/mapnik/expression.hpp
@@ -26,6 +26,7 @@
 // mapnik
 #include <mapnik/config.hpp>
 #include <mapnik/expression_node.hpp>
+#include <mapnik/expression_grammar.hpp>
 
 // stl
 #include <string>
@@ -33,20 +34,12 @@
 namespace mapnik
 {
 
-typedef boost::shared_ptr<expr_node> expression_ptr;
-template <typename Iterator> struct expression_grammar;
+typedef boost::shared_ptr<const expr_node> expression_ptr;
 
-class expression_factory
-{
-public:
-    static expression_ptr compile(std::string const& str,transcoder const& tr);
-    static bool parse_from_string(expression_ptr const& expr,
-                                  std::string const& str,
-                                  mapnik::expression_grammar<std::string::const_iterator> const& g);
-};
 
-MAPNIK_DECL expression_ptr parse_expression (std::string const& wkt, std::string const& encoding);
-MAPNIK_DECL expression_ptr parse_expression (std::string const& wkt);
+MAPNIK_DECL expression_ptr parse_expression (std::string const& wkt, std::string const& encoding = "UTF8");
+MAPNIK_DECL expression_ptr parse_expression (std::string const& wkt,
+											 mapnik::expression_grammar<std::string::const_iterator> const& g);
 
 }
 
diff --git a/src/expression.cpp b/src/expression.cpp
index eeafd7690..6cf9a4410 100644
--- a/src/expression.cpp
+++ b/src/expression.cpp
@@ -33,16 +33,26 @@
 namespace mapnik
 {
 
-expression_ptr expression_factory::compile(std::string const& str,transcoder const& tr)
+expression_ptr parse_expression(std::string const& str, std::string const& encoding)
 {
-    expression_ptr expr(boost::make_shared<expr_node>(true));
+    transcoder tr(encoding);
+    expression_grammar<std::string::const_iterator> g(tr);
+
+    return parse_expression(str, g);
+}
+
+expression_ptr parse_expression(std::string const& str,
+                                mapnik::expression_grammar<std::string::const_iterator> const& g)
+{
+    expr_node node;
+
     std::string::const_iterator itr = str.begin();
     std::string::const_iterator end = str.end();
-    mapnik::expression_grammar<std::string::const_iterator> g(tr);
-    bool r = boost::spirit::qi::phrase_parse(itr,end,g, boost::spirit::standard_wide::space,*expr);
+    
+    bool r = boost::spirit::qi::phrase_parse(itr, end, g, boost::spirit::standard_wide::space, node);
     if (r && itr==end)
     {
-        return expr;
+        return boost::make_shared<const expr_node>(node);
     }
     else
     {
@@ -50,24 +60,5 @@ expression_ptr expression_factory::compile(std::string const& str,transcoder con
     }
 }
 
-bool expression_factory::parse_from_string(expression_ptr const& expr,
-                                           std::string const& str,
-                                           mapnik::expression_grammar<std::string::const_iterator> const& g)
-{
-    std::string::const_iterator itr = str.begin();
-    std::string::const_iterator end = str.end();
-    bool r = boost::spirit::qi::phrase_parse(itr,end,g, boost::spirit::standard_wide::space,*expr);
-    return (r && itr==end);
-}
 
-expression_ptr parse_expression (std::string const& wkt,std::string const& encoding)
-{
-    transcoder tr(encoding);
-    return expression_factory::compile(wkt,tr);
-}
-
-expression_ptr parse_expression (std::string const& wkt)
-{
-    return parse_expression(wkt,"utf8");
-}
 }
diff --git a/src/xml_tree.cpp b/src/xml_tree.cpp
index 821f92ade..52a9bad58 100644
--- a/src/xml_tree.cpp
+++ b/src/xml_tree.cpp
@@ -100,15 +100,7 @@ inline boost::optional<color> fast_cast(xml_tree const& tree, std::string const&
 template <>
 inline boost::optional<expression_ptr> fast_cast(xml_tree const& tree, std::string const& value)
 {
-    expression_ptr expr(boost::make_shared<expr_node>(true));
-    if (expression_factory::parse_from_string(expr, value, tree.expr_grammar))
-    {
-        return expr;
-    }
-    else
-    {
-        throw mapnik::config_error("Failed to parse expression '" + value + "'");
-    }
+    return parse_expression(value, tree.expr_grammar);
 }
 
 /****************************************************************************/

From dc3763885cd67806823e316f073eb4359771b8b8 Mon Sep 17 00:00:00 2001
From: Colin Rundel <rundel@gmail.com>
Date: Wed, 29 Aug 2012 13:44:04 -0400
Subject: [PATCH 4/5] More parser clean up - color parser

Dropped color_factory class in favor of single color_parser function. Moved implementation to new color_factory.cpp since it is odd to have two headers (color.hpp, color_factory.hpp) and only one source file.
---
 demo/c++/rundemo.cpp                          |  2 +-
 include/mapnik/color.hpp                      | 52 +++++-----
 include/mapnik/color_factory.hpp              | 21 +---
 src/build.py                                  |  3 +-
 src/color.cpp                                 | 96 +------------------
 src/color_factory.cpp                         | 76 +++++++++++++++
 src/svg/svg_parser.cpp                        |  6 +-
 src/xml_tree.cpp                              | 10 +-
 .../svg_renderer_tests/combined_test.cpp      |  2 +-
 .../svg_renderer_tests/file_output_test.cpp   |  2 +-
 .../svg_renderer_tests/path_element_test.cpp  |  2 +-
 11 files changed, 120 insertions(+), 152 deletions(-)
 create mode 100644 src/color_factory.cpp

diff --git a/demo/c++/rundemo.cpp b/demo/c++/rundemo.cpp
index 31e016ced..1c918ea61 100644
--- a/demo/c++/rundemo.cpp
+++ b/demo/c++/rundemo.cpp
@@ -61,7 +61,7 @@ int main ( int argc , char** argv)
         freetype_engine::register_font(mapnik_dir + "/fonts/DejaVuSans.ttf");
 
         Map m(800,600);
-        m.set_background(color_factory::from_string("white"));
+        m.set_background(parse_color("white"));
 
         // create styles
 
diff --git a/include/mapnik/color.hpp b/include/mapnik/color.hpp
index ad9806276..6640086f8 100644
--- a/include/mapnik/color.hpp
+++ b/include/mapnik/color.hpp
@@ -47,37 +47,51 @@ private:
 
 public:
     color()
-        : red_(0xff),
+      : red_(0xff),
         green_(0xff),
         blue_(0xff),
         alpha_(0xff)
         {}
 
     color(unsigned red, unsigned green, unsigned blue, unsigned alpha = 0xff)
-        :  red_(red),
+      : red_(red),
         green_(green),
         blue_(blue),
         alpha_(alpha)
         {}
 
-    color( std::string const& css_string);
-
     color(const color& rhs)
-        : red_(rhs.red_),
+      : red_(rhs.red_),
         green_(rhs.green_),
         blue_(rhs.blue_),
         alpha_(rhs.alpha_)
         {}
 
-    color& operator=(const color& rhs)
-        {
-            if (this==&rhs) return *this;
-            red_=rhs.red_;
-            green_=rhs.green_;
-            blue_=rhs.blue_;
-            alpha_=rhs.alpha_;
+    color( std::string const& str);
+
+    std::string to_string() const;
+    std::string to_hex_string() const;
+
+    color& operator=(color const& rhs)
+    {
+        if (this==&rhs) 
             return *this;
-        }
+        
+        red_   = rhs.red_;
+        green_ = rhs.green_;
+        blue_  = rhs.blue_;
+        alpha_ = rhs.alpha_;
+        
+        return *this;
+    }
+
+    inline bool operator==(color const& rhs) const
+    {
+        return (red_== rhs.red()) &&
+               (green_ == rhs.green()) &&
+               (blue_  == rhs.blue()) &&
+               (alpha_ == rhs.alpha());
+    }
 
     inline unsigned red() const
     {
@@ -123,18 +137,6 @@ public:
         return (alpha_ << 24) | (blue_ << 16) | (green_ << 8) | (red_) ;
 #endif
     }
-
-    inline bool operator==(color const& rhs) const
-    {
-        return (red_== rhs.red()) &&
-            (green_ == rhs.green()) &&
-            (blue_  == rhs.blue()) &&
-            (alpha_ == rhs.alpha());
-
-    }
-
-    std::string to_string() const;
-    std::string to_hex_string() const;
 };
 
 template <typename charT, typename traits>
diff --git a/include/mapnik/color_factory.hpp b/include/mapnik/color_factory.hpp
index 418c244a0..312a4b3ec 100644
--- a/include/mapnik/color_factory.hpp
+++ b/include/mapnik/color_factory.hpp
@@ -24,27 +24,16 @@
 #define MAPNIK_COLOR_FACTORY_HPP
 
 // mapnik
-#include <mapnik/config.hpp>
+#include <mapnik/color.hpp>
+#include <mapnik/css_color_grammar.hpp>
 
-// boost
-#include <boost/utility.hpp>
+#include <string>
 
 namespace mapnik {
 
-class color;
+MAPNIK_DECL mapnik::color parse_color(std::string const& str);
+MAPNIK_DECL mapnik::color parse_color(std::string const& str, mapnik::css_color_grammar<std::string::const_iterator> const& g);
 
-template <typename Iterator> struct css_color_grammar;
-class MAPNIK_DECL color_factory : boost::noncopyable
-{
-public:
-
-    static void init_from_string(color & c, std::string const& css_color);
-
-    static bool parse_from_string(color & c, std::string const& css_color,
-                                  mapnik::css_color_grammar<std::string::const_iterator> const& g);
-
-    static color from_string(std::string const& css_color);
-};
 }
 
 #endif // MAPNIK_COLOR_FACTORY_HPP
diff --git a/src/build.py b/src/build.py
index 6a5162454..f11d0a243 100644
--- a/src/build.py
+++ b/src/build.py
@@ -187,6 +187,7 @@ source = Split(
     text_properties.cpp
     xml_tree.cpp
     config_error.cpp
+    color_factory.cpp
     """
     )
 
@@ -383,4 +384,4 @@ else:
     # delete in reverse order..
     env['create_uninstall_target'](env, target2)
     env['create_uninstall_target'](env, target1)
-    env['create_uninstall_target'](env, target)
\ No newline at end of file
+    env['create_uninstall_target'](env, target)
diff --git a/src/color.cpp b/src/color.cpp
index d48e9330c..d2db57384 100644
--- a/src/color.cpp
+++ b/src/color.cpp
@@ -27,22 +27,15 @@
 
 // boost
 #include <boost/format.hpp>
-#include <boost/version.hpp>
 
 // stl
 #include <sstream>
 
-#include <mapnik/css_color_grammar.hpp>
-
 namespace mapnik {
 
-color::color( std::string const& css_string)
-    : red_(0),
-      green_(0),
-      blue_(0),
-      alpha_(0xff)
+color::color(std::string const& str)
 {
-    color_factory::init_from_string(*this,css_string);
+    *this = parse_color(str);
 }
 
 std::string color::to_string() const
@@ -85,90 +78,5 @@ std::string color::to_hex_string() const
     }
 }
 
-
-/****************************************************************************/
-void color_factory::init_from_string(color & c, std::string const& css_color)
-{
-    typedef std::string::const_iterator iterator_type;
-    typedef mapnik::css_color_grammar<iterator_type> css_color_grammar;
-
-    css_color_grammar g;
-    iterator_type first = css_color.begin();
-    iterator_type last =  css_color.end();
-    // boost 1.41 -> 1.44 compatibility, to be removed in mapnik 2.1 (dane)
-#if BOOST_VERSION >= 104500
-    bool result =
-        boost::spirit::qi::phrase_parse(first,
-                                        last,
-                                        g,
-                                        boost::spirit::ascii::space,
-                                        c);
-    if (!result)
-    {
-        throw config_error(std::string("Failed to parse color value: ") +
-                           "Expected a CSS color, but got '" + css_color + "'");
-    }
-#else
-    mapnik::css css_;
-    bool result =
-        boost::spirit::qi::phrase_parse(first,
-                                        last,
-                                        g,
-                                        boost::spirit::ascii::space,
-                                        css_);
-    if (!result)
-    {
-        throw config_error(std::string("Failed to parse color value: ") +
-                           "Expected a CSS color, but got '" + css_color + "'");
-    }
-    c.set_red(css_.r);
-    c.set_green(css_.g);
-    c.set_blue(css_.b);
-    c.set_alpha(css_.a);
-#endif
-}
-
-bool color_factory::parse_from_string(color & c, std::string const& css_color,
-                                      mapnik::css_color_grammar<std::string::const_iterator> const& g)
-{
-    std::string::const_iterator first = css_color.begin();
-    std::string::const_iterator last =  css_color.end();
-    // boost 1.41 -> 1.44 compatibility, to be removed in mapnik 2.1 (dane)
-#if BOOST_VERSION >= 104500
-    bool result =
-        boost::spirit::qi::phrase_parse(first,
-                                        last,
-                                        g,
-                                        boost::spirit::ascii::space,
-                                        c);
-    return result && (first == last);
-#else
-    mapnik::css css_;
-    bool result =
-        boost::spirit::qi::phrase_parse(first,
-                                        last,
-                                        g,
-                                        boost::spirit::ascii::space,
-                                        css_);
-    if (result && (first == last))
-    {
-        c.set_red(css_.r);
-        c.set_green(css_.g);
-        c.set_blue(css_.b);
-        c.set_alpha(css_.a);
-        return true;
-    }
-    return false;
-#endif
-}
-
-
-color color_factory::from_string(std::string const& css_color)
-{
-    color c;
-    init_from_string(c, css_color);
-    return c;
-}
-
 }
 
diff --git a/src/color_factory.cpp b/src/color_factory.cpp
new file mode 100644
index 000000000..d9a1e6971
--- /dev/null
+++ b/src/color_factory.cpp
@@ -0,0 +1,76 @@
+/*****************************************************************************
+ *
+ * This file is part of Mapnik (c++ mapping toolkit)
+ *
+ * Copyright (C) 2011 Artem Pavlenko
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ *****************************************************************************/
+
+// mapnik
+#include <mapnik/color.hpp>
+#include <mapnik/color_factory.hpp>
+#include <mapnik/config_error.hpp>
+#include <mapnik/css_color_grammar.hpp>
+
+// boost
+#include <boost/format.hpp>
+#include <boost/version.hpp>
+
+// stl
+#include <sstream>
+
+
+namespace mapnik {
+
+color parse_color(std::string const& str)
+{
+    css_color_grammar<std::string::const_iterator> g;
+    return parse_color(str, g);
+}
+
+color parse_color(std::string const& str,
+                  css_color_grammar<std::string::const_iterator> const& g)
+{
+    color c;
+
+    std::string::const_iterator first = str.begin();
+    std::string::const_iterator last =  str.end();
+    
+    // boost 1.41 -> 1.44 compatibility, to be removed in mapnik 2.1 (dane)
+#if BOOST_VERSION >= 104500
+    bool result = boost::spirit::qi::phrase_parse(first, last, g,
+                                                  boost::spirit::ascii::space,
+                                                  c);
+#else
+    mapnik::css css_;
+    bool result = boost::spirit::qi::phrase_parse(first, last, g,
+                                                  boost::spirit::ascii::space, 
+                                                  css_);
+    c.set_red(css_.r);
+    c.set_green(css_.g);
+    c.set_blue(css_.b);
+    c.set_alpha(css_.a);
+    
+#endif
+
+    if (result && (first == last))
+        return c;
+    else
+        throw config_error( "Failed to parse color: \"" + str + "\"" );
+}
+
+}
diff --git a/src/svg/svg_parser.cpp b/src/svg/svg_parser.cpp
index 2cd3241dd..6b049ce63 100644
--- a/src/svg/svg_parser.cpp
+++ b/src/svg/svg_parser.cpp
@@ -74,7 +74,7 @@ agg::rgba8 parse_color(const char* str)
     mapnik::color c(100,100,100);
     try
     {
-        mapnik::color_factory::init_from_string(c,str);
+        c = mapnik::parse_color(str);
     }
     catch (mapnik::config_error & ex)
     {
@@ -784,7 +784,7 @@ void svg_parser::parse_gradient_stop(xmlTextReaderPtr reader)
             {
                 try
                 {
-                    mapnik::color_factory::init_from_string(stop_color,kv.second.c_str());
+                    stop_color = mapnik::parse_color(kv.second.c_str());
                 }
                 catch (mapnik::config_error & ex)
                 {
@@ -804,7 +804,7 @@ void svg_parser::parse_gradient_stop(xmlTextReaderPtr reader)
     {
         try
         {
-            mapnik::color_factory::init_from_string(stop_color,(const char *) value);
+            stop_color = mapnik::parse_color((const char *) value);
         }
         catch (mapnik::config_error & ex)
         {
diff --git a/src/xml_tree.cpp b/src/xml_tree.cpp
index 52a9bad58..c4d8cdcb8 100644
--- a/src/xml_tree.cpp
+++ b/src/xml_tree.cpp
@@ -86,15 +86,7 @@ inline boost::optional<std::string> fast_cast(xml_tree const& tree, std::string
 template <>
 inline boost::optional<color> fast_cast(xml_tree const& tree, std::string const& value)
 {
-    mapnik::color c;
-    if (mapnik::color_factory::parse_from_string(c, value, tree.color_grammar))
-    {
-        return c;
-    }
-    else
-    {
-        throw config_error("Failed to parse color '"+value+"'");
-    }
+    return parse_color(value, tree.color_grammar);
 }
 
 template <>
diff --git a/tests/cpp_tests/svg_renderer_tests/combined_test.cpp b/tests/cpp_tests/svg_renderer_tests/combined_test.cpp
index 34fec7504..ccc06a3e3 100644
--- a/tests/cpp_tests/svg_renderer_tests/combined_test.cpp
+++ b/tests/cpp_tests/svg_renderer_tests/combined_test.cpp
@@ -30,7 +30,7 @@ BOOST_AUTO_TEST_CASE(combined_test_case)
     typedef svg_renderer<std::ostream_iterator<char> > svg_ren;
 
     Map map(800, 600);
-    map.set_background(color_factory::from_string("white"));
+    map.set_background(parse_color("white"));
 
     std::ostringstream output_stream;
     std::ostream_iterator<char> output_stream_iterator(output_stream);
diff --git a/tests/cpp_tests/svg_renderer_tests/file_output_test.cpp b/tests/cpp_tests/svg_renderer_tests/file_output_test.cpp
index 8d468339e..bafaf2529 100644
--- a/tests/cpp_tests/svg_renderer_tests/file_output_test.cpp
+++ b/tests/cpp_tests/svg_renderer_tests/file_output_test.cpp
@@ -43,7 +43,7 @@ BOOST_AUTO_TEST_CASE(file_output_test_case)
     typedef svg_renderer<std::ostream_iterator<char> > svg_ren;
 
     Map map(800, 600);
-    map.set_background(color_factory::from_string("blue"));
+    map.set_background(parse_color("blue"));
 
     std::string output_filename = "file_output_test_case.svg";
     std::ofstream output_stream(output_filename.c_str());
diff --git a/tests/cpp_tests/svg_renderer_tests/path_element_test.cpp b/tests/cpp_tests/svg_renderer_tests/path_element_test.cpp
index 126d2e7a6..854b22930 100644
--- a/tests/cpp_tests/svg_renderer_tests/path_element_test.cpp
+++ b/tests/cpp_tests/svg_renderer_tests/path_element_test.cpp
@@ -220,7 +220,7 @@ void render_to_file(Map const& m, const std::string output_filename)
 BOOST_AUTO_TEST_CASE(path_element_test_case_1)
 {
     Map m(800,600);
-    m.set_background(color_factory::from_string("steelblue"));
+    m.set_background(parse_color("steelblue"));
 
     prepare_map(m);
 

From bd5df80f752925caf4a8e778f730001b93408ad6 Mon Sep 17 00:00:00 2001
From: Colin Rundel <rundel@gmail.com>
Date: Wed, 29 Aug 2012 15:35:48 -0400
Subject: [PATCH 5/5] Minimal roll back const changes

Changes to expression_ptr and path_expression_ptr are causing runtime issues with the python bindings
---
 include/mapnik/expression.hpp | 2 +-
 include/mapnik/parse_path.hpp | 2 +-
 src/expression.cpp            | 2 +-
 src/parse_path.cpp            | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/mapnik/expression.hpp b/include/mapnik/expression.hpp
index 7a8751c02..a6bdadd1c 100644
--- a/include/mapnik/expression.hpp
+++ b/include/mapnik/expression.hpp
@@ -34,7 +34,7 @@
 namespace mapnik
 {
 
-typedef boost::shared_ptr<const expr_node> expression_ptr;
+typedef boost::shared_ptr<expr_node> expression_ptr;
 
 
 MAPNIK_DECL expression_ptr parse_expression (std::string const& wkt, std::string const& encoding = "UTF8");
diff --git a/include/mapnik/parse_path.hpp b/include/mapnik/parse_path.hpp
index e89d9adfa..90b065ac9 100644
--- a/include/mapnik/parse_path.hpp
+++ b/include/mapnik/parse_path.hpp
@@ -41,7 +41,7 @@
 
 namespace mapnik {
 
-typedef boost::shared_ptr<const path_expression> path_expression_ptr;
+typedef boost::shared_ptr<path_expression> path_expression_ptr;
 
 MAPNIK_DECL path_expression_ptr parse_path(std::string const & str);
 MAPNIK_DECL path_expression_ptr parse_path(std::string const & str,
diff --git a/src/expression.cpp b/src/expression.cpp
index 6cf9a4410..e30505c70 100644
--- a/src/expression.cpp
+++ b/src/expression.cpp
@@ -52,7 +52,7 @@ expression_ptr parse_expression(std::string const& str,
     bool r = boost::spirit::qi::phrase_parse(itr, end, g, boost::spirit::standard_wide::space, node);
     if (r && itr==end)
     {
-        return boost::make_shared<const expr_node>(node);
+        return boost::make_shared<expr_node>(node);
     }
     else
     {
diff --git a/src/parse_path.cpp b/src/parse_path.cpp
index 96e1a4687..4d815e795 100644
--- a/src/parse_path.cpp
+++ b/src/parse_path.cpp
@@ -43,7 +43,7 @@ path_expression_ptr parse_path(std::string const& str,
     bool r = qi::phrase_parse(itr, end, g, boost::spirit::standard_wide::space, path);
     if (r  && itr == end)
     {
-        return boost::make_shared<const path_expression>(path); //path;
+        return boost::make_shared<path_expression>(path); //path;
     }
     else
     {