Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

fix/prevent to_expression_string misuse #985 #1232

Merged
merged 1 commit into from May 29, 2012

Conversation

Projects
None yet
2 participants
Contributor

lightmare commented May 29, 2012

A bit late I realized I could've split this in 2 commits...

  1. include/mapnik/rule.hpp and src/formatting/expression.cpp : fixed simple typos
  2. include/mapnik/expression_string.hpp : added to_expression_string overloads that, when instantiated, yield compilation error; and bindings/python/mapnik_expression.cpp consequently needed a small change

for example, with the new overloads and un-fixed include/mapnik/rule.hpp, I get this error from gcc:

include/mapnik/expression_string.hpp: In instantiation of ‘std::string mapnik::to_expression_string(const boost::shared_ptr<T>&) [with T = boost::variant<...>; std::string = std::basic_string<char>]’:
include/mapnik/rule.hpp +209 :73:   required from ‘void mapnik::rule::deepcopy_symbolizer::copy_height_ptr(T&) const [with T = mapnik::building_symbolizer]’
include/mapnik/rule.hpp +183 :32:   required from here
include/mapnik/expression_string.hpp +59 :59: error: cannot convert ‘mapnik::expr_node_ptr_’ to ‘mapnik::expr_node_ref_’ in initialization
include/mapnik/expression_string.hpp +59 :20: warning: unused variable ‘invalid_argument_type’ [-Wunused-variable]

couldn't get it any better than "cannot convert ptr to ref" :)

Owner

springmeyer commented May 29, 2012

awesome, thanks for this work!

@springmeyer springmeyer pushed a commit that referenced this pull request May 29, 2012

Dane Springmeyer Merge pull request #1232 from mirecta/issue-985
fix/prevent to_expression_string misuse #985
eaa53b0

@springmeyer springmeyer merged commit eaa53b0 into mapnik:master May 29, 2012

Owner

springmeyer commented May 29, 2012

hmm, does not compile with clang++ (I will look into fixing):


In file included from bindings/python/mapnik_expression.cpp:30:
include/mapnik/expression_string.hpp:51:20: error: cannot initialize a variable of type 'mapnik::expr_node_ref_' with an rvalue of type
      'mapnik::expr_node_ptr_'
    expr_node_ref_ invalid_argument_type = expr_node_ptr_();
                   ^                       ~~~~~~~~~~~~~~~~
include/mapnik/expression_string.hpp:59:20: error: cannot initialize a variable of type 'mapnik::expr_node_ref_' with an rvalue of type
      'mapnik::expr_node_ptr_'
    expr_node_ref_ invalid_argument_type = expr_node_ptr_();
                   ^                       ~~~~~~~~~~~~~~~~
2 errors generated.
Owner

springmeyer commented May 29, 2012

appears clang++ is instantiating these templates and finding their invalidity in the "first" pass: http://blog.llvm.org/2009/12/dreaded-two-phase-name-lookup.html. So, I think the invalid type needs to be a templated type to only throw upon explicit instanciation.

@springmeyer springmeyer pushed a commit that referenced this pull request May 29, 2012

Dane Springmeyer amend eaa53b0 and #1232 to compile with clang++ by avoiding first pas…
…s instanciation of intentionally invalid templates - refs #985
5feb975
Contributor

lightmare commented May 30, 2012

oops, okay I installed clang to check next time :)
in 5feb975 the raw pointer instantiation doesn't cause an error

I pushed another patch to https://github.com/mirecta/mapnik/commits/issue-985
it's simpler and produces better errors I think
(for the test I put an offending call at bindings/python/mapnik_expression.cpp:52)

gcc

include/mapnik/expression_string.hpp: In instantiation of ‘std::string mapnik::to_expression_string(const boost::shared_ptr<T>&) [with T = boost::variant<mapnik::value, mapnik::attribute, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::plus> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::minus> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::mult> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::div> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::mod> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::less> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::less_equal> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::greater> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::greater_equal> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::equal_to> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::not_equal_to> >, boost::recursive_wrapper<mapnik::unary_node<mapnik::tags::logical_not> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::logical_and> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::logical_or> >, boost::recursive_wrapper<mapnik::regex_match_node>, boost::recursive_wrapper<mapnik::regex_replace_node> >; std::string = std::basic_string<char>]’:
bindings/python/mapnik_expression.cpp +52 :58:   required from here
include/mapnik/expression_string.hpp +59 :12: error: could not convert ‘expr_node_ptr’ from ‘const boost::shared_ptr<boost::variant<mapnik::value, mapnik::attribute, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::plus> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::minus> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::mult> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::div> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::mod> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::less> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::less_equal> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::greater> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::greater_equal> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::equal_to> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::not_equal_to> >, boost::recursive_wrapper<mapnik::unary_node<mapnik::tags::logical_not> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::logical_and> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::logical_or> >, boost::recursive_wrapper<mapnik::regex_match_node>, boost::recursive_wrapper<mapnik::regex_replace_node> > >’ to ‘std::string {aka std::basic_string<char>}’

clang

include/mapnik/expression_string.hpp:59:12: error: no viable conversion from 'const boost::shared_ptr<variant<value, attribute, recursive_wrapper<binary_node<plus> >, recursive_wrapper<binary_node<minus> >,
      recursive_wrapper<binary_node<mult> >, recursive_wrapper<binary_node<div> >, recursive_wrapper<binary_node<mod> >, recursive_wrapper<binary_node<less> >, recursive_wrapper<binary_node<less_equal> >,
      recursive_wrapper<binary_node<greater> >, recursive_wrapper<binary_node<greater_equal> >, recursive_wrapper<binary_node<equal_to> >, recursive_wrapper<binary_node<not_equal_to> >,
      recursive_wrapper<unary_node<logical_not> >, recursive_wrapper<binary_node<logical_and> >, recursive_wrapper<binary_node<logical_or> >, recursive_wrapper<regex_match_node>,
      recursive_wrapper<regex_replace_node>, void_, void_> >' to 'std::string' (aka 'basic_string<char>')
    return expr_node_ptr; // to_expression_string() called with pointer argument
           ^~~~~~~~~~~~~
bindings/python/mapnik_expression.cpp:52:5: note: in instantiation of function template specialization 'mapnik::to_expression_string<boost::variant<mapnik::value, mapnik::attribute,
      boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::plus> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::minus> >,
      boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::mult> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::div> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::mod>
      >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::less> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::less_equal> >,
      boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::greater> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::greater_equal> >,
      boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::equal_to> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::not_equal_to> >,
      boost::recursive_wrapper<mapnik::unary_node<mapnik::tags::logical_not> >, boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::logical_and> >,
      boost::recursive_wrapper<mapnik::binary_node<mapnik::tags::logical_or> >, boost::recursive_wrapper<mapnik::regex_match_node>, boost::recursive_wrapper<mapnik::regex_replace_node>,
      boost::detail::variant::void_, boost::detail::variant::void_> >' requested here
    mapnik::to_expression_string(mapnik::expression_ptr());
    ^
Contributor

lightmare commented Jul 25, 2012

@springmeyer bump. The raw pointer overload currently in master doesn't break with x=0. In https://github.com/mirecta/mapnik/commits/issue-985 it does break when it should and compiles with gcc and clang (can't test msvc).

@springmeyer springmeyer pushed a commit that referenced this pull request Jul 25, 2012

Dane Springmeyer new patch from @lightmare for protecting against expression.to_string…
… misusage - closes #1232
b68ea3b
Owner

springmeyer commented Jul 25, 2012

thanks! tested on windows (that is compiles cleanly) and pushed in b68ea3b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment