From 77cd73325803155971f42a1301276cea43816b3b Mon Sep 17 00:00:00 2001
From: Artem Pavlenko <artem@mapnik.org>
Date: Tue, 30 Apr 2024 09:59:35 +0100
Subject: [PATCH 1/3] svg_group - check parent is not nullptr

---
 include/mapnik/svg/svg_converter.hpp | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/mapnik/svg/svg_converter.hpp b/include/mapnik/svg/svg_converter.hpp
index 103a14d6c..0bb09b6bc 100644
--- a/include/mapnik/svg/svg_converter.hpp
+++ b/include/mapnik/svg/svg_converter.hpp
@@ -73,7 +73,11 @@ class svg_converter : util::noncopyable
         current_group_ = &current_group_->elements.back().get<group>();
     }
 
-    void end_group() { current_group_ = current_group_->parent; }
+    void end_group()
+    {
+        if (current_group_->parent != nullptr)
+            current_group_ = current_group_->parent;
+    }
 
     void begin_path()
     {

From e412e125995a72ecbe2cbc685c5ba820021e929b Mon Sep 17 00:00:00 2001
From: Artem Pavlenko <artem@mapnik.org>
Date: Tue, 30 Apr 2024 10:01:02 +0100
Subject: [PATCH 2/3] svg_parser - cleanup + consistent push/pop attributes

---
 src/svg/svg_parser.cpp | 216 +++++++++++++++++++----------------------
 1 file changed, 98 insertions(+), 118 deletions(-)

diff --git a/src/svg/svg_parser.cpp b/src/svg/svg_parser.cpp
index c67cf4466..853438fd3 100644
--- a/src/svg/svg_parser.cpp
+++ b/src/svg/svg_parser.cpp
@@ -500,117 +500,102 @@ void traverse_tree(svg_parser& parser, rapidxml::xml_node<char> const* node)
 
     switch (node->type())
     {
-        case rapidxml::node_element: {
-            parser.font_sizes_.push_back(parser.font_sizes_.back());
-            switch (name)
+    case rapidxml::node_element:
+        parser.font_sizes_.push_back(parser.font_sizes_.back());
+        switch (name)
+        {
+        case "defs"_case: {
+            if (node->first_node() != nullptr)
             {
-                case "defs"_case: {
-                    if (node->first_node() != nullptr)
-                    {
-                        parser.is_defs_ = true;
-                    }
-                    break;
-                }
-                case "clipPath"_case:
-                case "symbol"_case:
-                case "pattern"_case: {
-                    parser.ignore_ = true;
-                    break;
-                }
-                // the gradient tags *should* be in defs, but illustrator seems not to put them in there so
-                // accept them anywhere
-                case "linearGradient"_case:
-                    parse_linear_gradient(parser, node);
-                    break;
-                case "radialGradient"_case:
-                    parse_radial_gradient(parser, node);
-                    break;
+                parser.is_defs_ = true;
             }
-
-            if (!parser.is_defs_) // FIXME
-            {
-                switch (name)
-                {
-                    case "g"_case:
-                        if (node->first_node() != nullptr)
-                        {
-                            parser.path_.push_attr();
-                            parse_id(parser, node);
-                            if (parser.css_style_)
-                                process_css(parser, node);
-                            parse_attr(parser, node);
-                            parser.path_.begin_group();
-                        }
-                        break;
-                    case "use"_case:
-                        parser.path_.push_attr();
-                        parse_id(parser, node);
-                        if (parser.css_style_)
-                            process_css(parser, node);
-                        parse_attr(parser, node);
-                        if (parser.path_.cur_attr().opacity < 1.0)
-                            parser.path_.begin_group();
-                        parse_use(parser, node);
-                        if (parser.path_.cur_attr().opacity < 1.0)
-                            parser.path_.end_group();
-                        parser.path_.pop_attr();
-                        break;
-                    default:
-                        parser.path_.push_attr();
-                        parse_id(parser, node);
-                        if (parser.css_style_)
-                            process_css(parser, node);
-                        parse_attr(parser, node);
-                        if (parser.path_.display())
-                        {
-                            if (parser.path_.cur_attr().opacity < 1.0)
-                                parser.path_.begin_group();
-                            parse_element(parser, node->name(), node);
-                            if (parser.path_.cur_attr().opacity < 1.0)
-                                parser.path_.end_group();
-                        }
-                        parser.path_.pop_attr();
-                }
-            }
-            else
-            {
-                // save node for later
-                parse_id(parser, node);
-            }
-
-            if ("style"_case == name)
-            {
-                // <style> element is not expected to have nested elements
-                // we're only interested in DATA or CDATA
-                for (auto const* child = node->first_node(); child; child = child->next_sibling())
-                {
-                    if (child->type() == rapidxml::node_data || child->type() == rapidxml::node_cdata)
-                    {
-                        auto const grammar = mapnik::grammar();
-                        auto const skipper = mapnik::skipper();
-                        char const* first = child->value();
-                        char const* last = first + child->value_size();
-                        bool result = boost::spirit::x3::phrase_parse(first, last, grammar, skipper, parser.css_data_);
-                        if (result && first == last && !parser.css_data_.empty())
-                        {
-                            parser.css_style_ = true;
-                        }
-                    }
-                }
-            }
-            else
-            {
-                for (auto const* child = node->first_node(); child; child = child->next_sibling())
-                {
-                    traverse_tree(parser, child);
-                }
-            }
-
-            end_element(parser, node);
-        }
-        break;
-        default:
             break;
+        }
+        case "clipPath"_case:
+        case "symbol"_case:
+        case "pattern"_case: {
+            parser.ignore_ = true;
+            break;
+        }
+        // the gradient tags *should* be in defs, but illustrator seems not to put them in there so
+        // accept them anywhere
+        case "linearGradient"_case:
+            parse_linear_gradient(parser, node);
+            break;
+        case "radialGradient"_case:
+            parse_radial_gradient(parser, node);
+            break;
+        case "g"_case:
+            parser.path_.push_attr();
+            parse_id(parser, node);
+            if (parser.css_style_)
+                process_css(parser, node);
+            parse_attr(parser, node);
+            parser.path_.begin_group();
+            break;
+        case "use"_case:
+            parser.path_.push_attr();
+            parse_id(parser, node);
+            if (parser.css_style_)
+                process_css(parser, node);
+            parse_attr(parser, node);
+            if (parser.path_.cur_attr().opacity < 1.0)
+                parser.path_.begin_group();
+            parse_use(parser, node);
+            if (parser.path_.cur_attr().opacity < 1.0)
+                parser.path_.end_group();
+            parser.path_.pop_attr();
+            break;
+        default:
+            parser.path_.push_attr();
+            parse_id(parser, node);
+            if (parser.css_style_)
+                process_css(parser, node);
+            parse_attr(parser, node);
+            if (parser.path_.display())
+            {
+                if (parser.path_.cur_attr().opacity < 1.0)
+                    parser.path_.begin_group();
+                parse_element(parser, node->name(), node);
+                if (parser.path_.cur_attr().opacity < 1.0)
+                    parser.path_.end_group();
+            }
+            parser.path_.pop_attr();
+            break;
+        }
+
+        if ("style"_case == name)
+        {
+            // <style> element is not expected to have nested elements
+            // we're only interested in DATA or CDATA
+            for (auto const* child = node->first_node(); child; child = child->next_sibling())
+            {
+                if (child->type() == rapidxml::node_data || child->type() == rapidxml::node_cdata)
+                {
+                    auto const grammar = mapnik::grammar();
+                    auto const skipper = mapnik::skipper();
+                    char const* first = child->value();
+                    char const* last = first + child->value_size();
+                    bool result = boost::spirit::x3::phrase_parse(first, last, grammar, skipper, parser.css_data_);
+                    if (result && first == last && !parser.css_data_.empty())
+                    {
+                        parser.css_style_ = true;
+                    }
+                }
+            }
+        }
+        else
+        {
+            for (auto const* child = node->first_node(); child; child = child->next_sibling())
+            {
+                traverse_tree(parser, child);
+            }
+        }
+
+        end_element(parser, node);
+        break;
+    default:
+        break;
     }
 }
 
@@ -618,20 +603,15 @@ void end_element(svg_parser& parser, rapidxml::xml_node<char> const* node)
 {
     parser.font_sizes_.pop_back();
     auto name = name_to_int(node->name());
-    if (!parser.is_defs_ && (name == "g"_case))
+    if (name == "g"_case)
     {
         parser.path_.end_group();
-        if (node->first_node() != nullptr)
-        {
-            parser.path_.pop_attr();
-        }
+        parser.path_.pop_attr();
+
     }
     else if (name == "svg"_case)
     {
-        // if (node->first_node() != nullptr)
-        //{
-        // parser.path_.pop_attr();
-        //}
+        parser.path_.pop_attr();
     }
     else if (name == "defs"_case)
     {
@@ -680,7 +660,7 @@ void parse_element(svg_parser& parser, char const* name, rapidxml::xml_node<char
             parse_ellipse(parser, node);
             break;
         case "svg"_case:
-            // parser.path_.push_attr();
+            parser.path_.push_attr();
             parse_dimensions(parser, node);
             parse_attr(parser, node);
             parser.path_.set_opacity(parser.path_.cur_attr().opacity);

From 3176749613e09a50e178798414478073f400c611 Mon Sep 17 00:00:00 2001
From: Artem Pavlenko <artem@mapnik.org>
Date: Tue, 30 Apr 2024 10:04:57 +0100
Subject: [PATCH 3/3] clang-format (pre-commit run --show-diff-on-failure
 --color=always --all-files)

---
 src/svg/svg_parser.cpp | 177 ++++++++++++++++++++---------------------
 1 file changed, 88 insertions(+), 89 deletions(-)

diff --git a/src/svg/svg_parser.cpp b/src/svg/svg_parser.cpp
index 853438fd3..1f08a7256 100644
--- a/src/svg/svg_parser.cpp
+++ b/src/svg/svg_parser.cpp
@@ -500,102 +500,102 @@ void traverse_tree(svg_parser& parser, rapidxml::xml_node<char> const* node)
 
     switch (node->type())
     {
-    case rapidxml::node_element:
-        parser.font_sizes_.push_back(parser.font_sizes_.back());
-        switch (name)
-        {
-        case "defs"_case: {
-            if (node->first_node() != nullptr)
+        case rapidxml::node_element:
+            parser.font_sizes_.push_back(parser.font_sizes_.back());
+            switch (name)
             {
-                parser.is_defs_ = true;
-            }
-            break;
-        }
-        case "clipPath"_case:
-        case "symbol"_case:
-        case "pattern"_case: {
-            parser.ignore_ = true;
-            break;
-        }
-        // the gradient tags *should* be in defs, but illustrator seems not to put them in there so
-        // accept them anywhere
-        case "linearGradient"_case:
-            parse_linear_gradient(parser, node);
-            break;
-        case "radialGradient"_case:
-            parse_radial_gradient(parser, node);
-            break;
-        case "g"_case:
-            parser.path_.push_attr();
-            parse_id(parser, node);
-            if (parser.css_style_)
-                process_css(parser, node);
-            parse_attr(parser, node);
-            parser.path_.begin_group();
-            break;
-        case "use"_case:
-            parser.path_.push_attr();
-            parse_id(parser, node);
-            if (parser.css_style_)
-                process_css(parser, node);
-            parse_attr(parser, node);
-            if (parser.path_.cur_attr().opacity < 1.0)
-                parser.path_.begin_group();
-            parse_use(parser, node);
-            if (parser.path_.cur_attr().opacity < 1.0)
-                parser.path_.end_group();
-            parser.path_.pop_attr();
-            break;
-        default:
-            parser.path_.push_attr();
-            parse_id(parser, node);
-            if (parser.css_style_)
-                process_css(parser, node);
-            parse_attr(parser, node);
-            if (parser.path_.display())
-            {
-                if (parser.path_.cur_attr().opacity < 1.0)
-                    parser.path_.begin_group();
-                parse_element(parser, node->name(), node);
-                if (parser.path_.cur_attr().opacity < 1.0)
-                    parser.path_.end_group();
-            }
-            parser.path_.pop_attr();
-            break;
-        }
-
-        if ("style"_case == name)
-        {
-            // <style> element is not expected to have nested elements
-            // we're only interested in DATA or CDATA
-            for (auto const* child = node->first_node(); child; child = child->next_sibling())
-            {
-                if (child->type() == rapidxml::node_data || child->type() == rapidxml::node_cdata)
-                {
-                    auto const grammar = mapnik::grammar();
-                    auto const skipper = mapnik::skipper();
-                    char const* first = child->value();
-                    char const* last = first + child->value_size();
-                    bool result = boost::spirit::x3::phrase_parse(first, last, grammar, skipper, parser.css_data_);
-                    if (result && first == last && !parser.css_data_.empty())
+                case "defs"_case: {
+                    if (node->first_node() != nullptr)
                     {
-                        parser.css_style_ = true;
+                        parser.is_defs_ = true;
+                    }
+                    break;
+                }
+                case "clipPath"_case:
+                case "symbol"_case:
+                case "pattern"_case: {
+                    parser.ignore_ = true;
+                    break;
+                }
+                // the gradient tags *should* be in defs, but illustrator seems not to put them in there so
+                // accept them anywhere
+                case "linearGradient"_case:
+                    parse_linear_gradient(parser, node);
+                    break;
+                case "radialGradient"_case:
+                    parse_radial_gradient(parser, node);
+                    break;
+                case "g"_case:
+                    parser.path_.push_attr();
+                    parse_id(parser, node);
+                    if (parser.css_style_)
+                        process_css(parser, node);
+                    parse_attr(parser, node);
+                    parser.path_.begin_group();
+                    break;
+                case "use"_case:
+                    parser.path_.push_attr();
+                    parse_id(parser, node);
+                    if (parser.css_style_)
+                        process_css(parser, node);
+                    parse_attr(parser, node);
+                    if (parser.path_.cur_attr().opacity < 1.0)
+                        parser.path_.begin_group();
+                    parse_use(parser, node);
+                    if (parser.path_.cur_attr().opacity < 1.0)
+                        parser.path_.end_group();
+                    parser.path_.pop_attr();
+                    break;
+                default:
+                    parser.path_.push_attr();
+                    parse_id(parser, node);
+                    if (parser.css_style_)
+                        process_css(parser, node);
+                    parse_attr(parser, node);
+                    if (parser.path_.display())
+                    {
+                        if (parser.path_.cur_attr().opacity < 1.0)
+                            parser.path_.begin_group();
+                        parse_element(parser, node->name(), node);
+                        if (parser.path_.cur_attr().opacity < 1.0)
+                            parser.path_.end_group();
+                    }
+                    parser.path_.pop_attr();
+                    break;
+            }
+
+            if ("style"_case == name)
+            {
+                // <style> element is not expected to have nested elements
+                // we're only interested in DATA or CDATA
+                for (auto const* child = node->first_node(); child; child = child->next_sibling())
+                {
+                    if (child->type() == rapidxml::node_data || child->type() == rapidxml::node_cdata)
+                    {
+                        auto const grammar = mapnik::grammar();
+                        auto const skipper = mapnik::skipper();
+                        char const* first = child->value();
+                        char const* last = first + child->value_size();
+                        bool result = boost::spirit::x3::phrase_parse(first, last, grammar, skipper, parser.css_data_);
+                        if (result && first == last && !parser.css_data_.empty())
+                        {
+                            parser.css_style_ = true;
+                        }
                     }
                 }
             }
-        }
-        else
-        {
-            for (auto const* child = node->first_node(); child; child = child->next_sibling())
+            else
             {
-                traverse_tree(parser, child);
+                for (auto const* child = node->first_node(); child; child = child->next_sibling())
+                {
+                    traverse_tree(parser, child);
+                }
             }
-        }
 
-        end_element(parser, node);
-        break;
-    default:
-        break;
+            end_element(parser, node);
+            break;
+        default:
+            break;
     }
 }
 
@@ -607,7 +607,6 @@ void end_element(svg_parser& parser, rapidxml::xml_node<char> const* node)
     {
         parser.path_.end_group();
         parser.path_.pop_attr();
-
     }
     else if (name == "svg"_case)
     {