Merge pull request #1232 from mirecta/issue-985

fix/prevent to_expression_string misuse #985

bindings/python/mapnik_expression.cpp

@@ -46,6 +46,11 @@ expression_ptr parse_expression_(std::string const& wkt)
     return parse_expression(wkt,"utf8");
 }
 
+std::string expression_to_string_(mapnik::expr_node const& expr)
+{
+    return mapnik::to_expression_string(expr);
+}
+
 mapnik::value expression_evaluate_(mapnik::expr_node const& expr, mapnik::Feature const& f)
 {
     // will be auto-converted to proper python type by `mapnik_value_to_python`
@@ -75,7 +80,7 @@ void export_expression()
                                                   "TODO"
                                                   "",no_init)
         .def("evaluate", &expression_evaluate_)
-        .def("__str__",&to_expression_string);
+        .def("__str__",&expression_to_string_);
     ;
 
     def("Expression",&parse_expression_,(arg("expr")),"Expression string");

include/mapnik/expression_string.hpp

@@ -33,6 +33,33 @@
 namespace mapnik
 {
 MAPNIK_DECL std::string to_expression_string(expr_node const& node);
+
+// Dummy types that are used to trigger nice (with gcc at least)
+// compilation error when to_expression_string is misused.
+enum expr_node_ref_ {};
+enum expr_node_ptr_ {};
+
+// The following two templates should prevent accidentally passing
+// a pointer (either raw or shared) as the argument.  Without them,
+// the compiler would construct a temporary expr_node(bool) using
+// implicit pointer-to-bool conversion, thus any non-null pointer
+// would yield "true".
+
+template <typename T>
+std::string to_expression_string(T const* x)
+{
+    expr_node_ref_ invalid_argument_type = expr_node_ptr_();
+    throw std::logic_error("to_expression_string() called with pointer argument");
+    return std::string();
+}
+
+template <typename T>
+std::string to_expression_string(boost::shared_ptr<T> const& x)
+{
+    expr_node_ref_ invalid_argument_type = expr_node_ptr_();
+    throw std::logic_error("to_expression_string() called with pointer argument");
+    return std::string();
+}
 }
 
 #endif // MAPNIK_EXPRESSION_STRING_HPP

include/mapnik/rule.hpp

@@ -206,7 +206,7 @@ private:
         template <class T>
         void copy_height_ptr(T & sym) const
         {
-            std::string height_expr = to_expression_string(sym.height());
+            std::string height_expr = to_expression_string(*sym.height());
             sym.set_height(parse_expression(height_expr,"utf8"));
         }
     };

src/formatting/expression.cpp

@@ -40,7 +40,7 @@ void expression_format::to_xml(boost::property_tree::ptree &xml) const
     ptree &new_node = xml.push_back(ptree::value_type("ExpressionFormat", ptree()))->second;
     if (face_name) set_attr(new_node, "face-name", to_expression_string(*face_name));
     if (text_size) set_attr(new_node, "size", to_expression_string(*text_size));
-    if (character_spacing) set_attr(new_node, "character-spacing", to_expression_string*character_spacing);
+    if (character_spacing) set_attr(new_node, "character-spacing", to_expression_string(*character_spacing));
     if (line_spacing) set_attr(new_node, "line-spacing", to_expression_string(*line_spacing));
     if (text_opacity) set_attr(new_node, "opacity", to_expression_string(*text_opacity));
     if (wrap_before) set_attr(new_node, "wrap-before", to_expression_string(*wrap_before));