Mapnik Compilation warning C4172: returning address of local variable or temporary?

[jc101]

Has anyone experienced these warnings around compiling the color files (eg css_color_grammar.cpp; color.cpp) when building Mapnik?

warning C4172: returning address of local variable or temporary

They seem to occur in boost phoenix header files eg meta_grammar.hpp,actor_operator_10.hpp etc.

I suspect (following discussions over on mapnik) that they are causing access violation errors (when inline functions are enabled - mangled strings when disabled) I am experiencing with outputting the color to a hex string using the karma generator but am just wondering if other people experience these warnings too?

[springmeyer]

Yes, I've seen these warnings. Because they are coming out of boost (not mapnik) I had not paid much attention, but yes we should take a closer look to make sure they are not happening due to Mapnik usage. /cc @artemp

[lightmare]

Do these occur when karma attributes are assigned references instead of temporaries?

diff --git a/src/color.cpp b/src/color.cpp
index 2dcbe76..2a35353 100644
--- a/src/color.cpp
+++ b/src/color.cpp
@@ -85,10 +85,10 @@ std::string color::to_hex_string() const
     karma::generate(sink,
                     // begin grammar
                     '#'
-                    << right_align(2,'0')[hex[_1 = red()]]
-                    << right_align(2,'0')[hex[_1 = green()]]
-                    << right_align(2,'0')[hex[_1 = blue()]]
-                    << eps(alpha() < 255) <<  right_align(2,'0')[hex [_1 = alpha()]]
+                    << right_align(2,'0')[hex[_1 = red_]]
+                    << right_align(2,'0')[hex[_1 = green_]]
+                    << right_align(2,'0')[hex[_1 = blue_]]
+                    << eps(alpha() < 255) <<  right_align(2,'0')[hex [_1 = alpha_]]
                     // end grammar
         );
     return str;

[jc101]

I recently left a reply on the mapnik issues thread. I think I directly caused the error when I was modifying a patch. I had left in a BOOST_SPIRIT_UNICODE pragma in value.hpp which, when taken out, stopped the access violation when inlining was renabled and the garbled strings when it wasn't.

I had previously tried a number of things including what you suggested but nothing seemed to stop the error, It was only when I recompared my changes to the current codebase that I realised the extra pragma was still there.

I haven't closed the issue as the compiler warnings are still there even though they don't seem to have caused my problem. If it's not seen as a big deal though I don't mind if it is closed.

[lightmare]

I'm asking because I'm not sure what exactly [_1 = red()] does. If it copies the value, then fine. If it binds a reference to it, then fine, standard says reference extends the lifetime of the temporary; but it might trigger the warning, and in that case we can get rid of it by using red_ instead. If it stores a pointer to the rvalue, then that is a problem; but I think if that was the case, you'd see bogus output.

Anyway, I will probably rewrite at least color::to_hex_string, karma is plain overkill for something that could be 5 lines of C.

[springmeyer]

Anyway, I will probably rewrite at least color::to_hex_string, karma is plain overkill for something that could be 5 lines of C.

@lightmare @artemp might complain, but I'm a big 👍 :) That would allow us to drop compile time for color.cpp significantly

[artemp]

@springmeyer - not so quick :) . Remember, mapnik is C++ not C !!
@lightmare

Anyway, I will probably rewrite at least color::to_hex_string, karma is plain overkill for something that could be 5 lines of C.

What? I'm not even mildly convinced by your statement but I'd like to see your 5 lines in C proposal
ref : http://www.boost.org/doc/libs/1_60_0/libs/spirit/doc/html/spirit/karma/tutorials/quick_start.html

[artemp]

@lightmare @springmeyer - I think colour generators don't need a phoenix dust, I'll take a look at simplifying.

[jc101]

For what it's worth, while I was trying to figure out what was causing my problem I did rewrite the color::to_hex_string function. It's not 5 lines but it is less than 10 and seems to work :)

std::string color::to_hex_string() const
{
    std::stringstream stream;
    stream << "#";
    stream << std::setfill('0') << std::setw(2) << std::hex << 0u + red(); // implicit cast to unsigned int before hex conversion
    stream << std::setfill('0') << std::setw(2) << std::hex << 0u + green();
    stream << std::setfill('0') << std::setw(2) << std::hex << 0u + blue();
    if (alpha() < 255) {
        stream << std::setfill('0') << std::setw(2) << std::hex << 0u + alpha();
    }
    return stream.str();
}

[artemp]

@jc101 - the approach above works but I prefer boost::karma :)

could you try mapnik/mapnik@3e8ee9a and see if warning are gone, pls ?

[lightmare]

I'd like to see your 5 lines in C proposal

🎯 touche! That was unfairly exaggerated, not sure why I imagined color stored as uint8_t[3] and no alpha, that would be possible in 7 lines (almost C 🍰 )

    static char const xdigits[] = "0123456789abcdef";
    char buf[7] = {'#'};
    for (int i = 0; i < 3; ++i) {
        buf[1 + 2 * i] = xdigits[(c[i] >> 4) & 15];
        buf[2 + 2 * i] = xdigits[c[i] & 15];
    }
    return std::string(buf, 7);

Here's a real, 15-line body version

std::string color::to_hex_string() const
{
    static char const xdigits[] = "0123456789abcdef";
    char buf[9];
    buf[0] = '#';
    buf[1] = xdigits[(red_ >> 4) & 15]; 
    buf[2] = xdigits[red_ & 15];
    buf[3] = xdigits[(green_ >> 4) & 15]; 
    buf[4] = xdigits[green_ & 15];
    buf[5] = xdigits[(blue_ >> 4) & 15]; 
    buf[6] = xdigits[blue_ & 15];
    if (alpha_ == 255) {
        return std::string(buf, 7);
    }
    buf[7] = xdigits[(alpha_ >> 4) & 15];
    buf[8] = xdigits[alpha_ & 15];
    return std::string(buf, 9); 
}

[artemp]

@lightmare - 👏 you get 🏆 😺 ! On a serious note - I still prefer to use boost::spirit for parsing/generating output in mapnik rather than resorting to handwriting parser/generators. I'm thinking long term stability, readability, consistency etc etc.

[artemp]

@lightmare - here is a real challenge => apply_markers_multi

src/renderer_common/render_markers_symbolizer.cpp

Command execution time: src/renderer_common/render_markers_symbolizer.os: 136.205251 seconds

It would be super awesome to fix that ^. I know you have looked already but anything else we can do to improve? In any case should we at least split void operator() (T const& mark) const into separate *.cpp ?

[lightmare]

I still prefer to use boost::spirit for parsing/generating output in mapnik rather than resorting to handwriting parser/generators.

I've been pondering handwriting spirit::qi::parser hexcolor for a few weeks, as I don't find the grammar particularly readable. I think hexcolor should be a single token in the grammar; whether written in spirit, handwritten or re2c generated, doesn't matter; but it's something so small and rigid that I prefer a handwritten parser that compiles in no time. Complex spirit grammars are fun 🎢 Grammar for 3, 4, 6 or 8 hex digits... not for me 😜

src/renderer_common/render_markers_symbolizer.cpp

Yea, I stopped there. Separating the operators might help, as they require different apply_markers_multi overloads; haven't tried that, though.

[jc101]

@artemp I tried the changes you made and the warnings are gone; fyi they still exist on two files: css_color_grammar.cpp and css_color.cpp.