extract_uri: fix, test, document

9 files changed, 139 insertions, 141 deletions

diff --git a/src/extract-uri.cpp b/src/extract-uri.cpp
index 228e0c1c9..65401df6e 100644
--- a/src/extract-uri.cpp
+++ b/src/extract-uri.cpp
@@ -16,15 +16,16 @@
 
 // Functions as per 4.3.4 of CSS 2.1
 // http://www.w3.org/TR/CSS21/syndata.html#uri
-gchar *extract_uri( gchar const *s, gchar const** endptr )
+std::string extract_uri(char const *s, char const **endptr)
 {
+    std::string result;
+
     if (!s)
-        return nullptr;
+        return result;
 
-    gchar* result = nullptr;
     gchar const *sb = s;
     if ( strlen(sb) < 4 || strncmp(sb, "url", 3) != 0 ) {
-        return nullptr;
+        return result;
     }
 
     sb += 3;
@@ -54,7 +55,12 @@ gchar *extract_uri( gchar const *s, gchar const** endptr )
             delim = *sb;
             sb++;
         }
-        gchar const* se = sb + 1;
+
+        if (!*sb) {
+            return result;
+        }
+
+        gchar const* se = sb;
         while ( *se && (*se != delim) ) {
             se++;
         }
@@ -67,14 +73,12 @@ gchar *extract_uri( gchar const *s, gchar const** endptr )
                 }
 
                 // back up for any trailing whitespace
-                se--;
-                while ( ( se[-1] == ' ' ) ||
-                        ( se[-1] == '\t' ) )
+                while (se > sb && g_ascii_isspace(se[-1]))
                 {
                     se--;
                 }
 
-                result = g_strndup(sb, se - sb + 1);
+                result = std::string(sb, se);
             } else {
                 gchar const* tail = se + 1;
                 while ( ( *tail == ' ' ) ||
@@ -86,7 +90,7 @@ gchar *extract_uri( gchar const *s, gchar const** endptr )
                     if ( endptr ) {
                         *endptr = tail + 1;
                     }
-                    result = g_strndup(sb, se - sb);
+                    result = std::string(sb, se);
                 }
             }
         }
diff --git a/src/extract-uri.h b/src/extract-uri.h
index a4de3c856..ee773f458 100644
--- a/src/extract-uri.h
+++ b/src/extract-uri.h
@@ -10,7 +10,31 @@
 #ifndef SEEN_EXTRACT_URI_H
 #define SEEN_EXTRACT_URI_H
 
-char *extract_uri(char const *s, char const** endptr = nullptr);
+#include <string>
+
+/**
+ * Parse functional URI notation, as per 4.3.4 of CSS 2.1
+ *
+ * http://www.w3.org/TR/CSS21/syndata.html#uri
+ *
+ * > The format of a URI value is 'url(' followed by optional white space
+ * > followed by an optional single quote (') or double quote (") character
+ * > followed by the URI itself, followed by an optional single quote (')
+ * > or double quote (") character followed by optional white space
+ * > followed by ')'. The two quote characters must be the same.
+ *
+ * Example:
+ * \verbatim
+   url = extract_uri("url('foo')bar", &out);
+   -> url == "foo"
+   -> out == "bar"
+   \endverbatim
+ *
+ * @param s String which starts with "url("
+ * @param[out] endptr points to \c s + N, where N is the number of characters parsed
+ * @return URL string, or empty string on failure
+ */
+std::string extract_uri(char const *s, char const **endptr = nullptr);
 
 
 #endif /* !SEEN_EXTRACT_URI_H */
diff --git a/src/id-clash.cpp b/src/id-clash.cpp
index 2ee358d49..98c693e50 100644
--- a/src/id-clash.cpp
+++ b/src/id-clash.cpp
@@ -107,12 +107,11 @@ find_references(SPObject *elem, refmap_type &refmap)
                 const char *attr = clipboard_properties[i];
                 const gchar *value = sp_repr_css_property(css, attr, nullptr);
                 if (value) {
-                    gchar *uri = extract_uri(value);
-                    if (uri && uri[0] == '#') {
+                    auto uri = extract_uri(value);
+                    if (uri[0] == '#') {
                         IdReference idref = { REF_CLIPBOARD, elem, attr };
-                        refmap[uri+1].push_back(idref);
+                        refmap[uri.c_str() + 1].push_back(idref);
                     }
-                    g_free(uri);
                 }
             }
 
@@ -163,12 +162,11 @@ find_references(SPObject *elem, refmap_type &refmap)
     for (unsigned i = SP_MARKER_LOC_START; i < SP_MARKER_LOC_QTY; i++) {
         const gchar *value = style->marker_ptrs[i]->value;
         if (value) {
-            gchar *uri = extract_uri(value);
-            if (uri && uri[0] == '#') {
+            auto uri = extract_uri(value);
+            if (uri[0] == '#') {
                 IdReference idref = { REF_STYLE, elem, markers[i] };
-                refmap[uri+1].push_back(idref);
+                refmap[uri.c_str() + 1].push_back(idref);
             }
-            g_free(uri);
         }
     }
 
@@ -177,12 +175,11 @@ find_references(SPObject *elem, refmap_type &refmap)
         const char *attr = other_url_properties[i];
         const gchar *value = repr_elem->attribute(attr);
         if (value) {
-            gchar *uri = extract_uri(value);
-            if (uri && uri[0] == '#') {
+            auto uri = extract_uri(value);
+            if (uri[0] == '#') {
                 IdReference idref = { REF_URL, elem, attr };
-                refmap[uri+1].push_back(idref);
+                refmap[uri.c_str() + 1].push_back(idref);
             }
-            g_free(uri);
         }
     }
     
diff --git a/src/object/sp-item.cpp b/src/object/sp-item.cpp
index d6d1bdf56..edf216481 100644
--- a/src/object/sp-item.cpp
+++ b/src/object/sp-item.cpp
@@ -441,15 +441,14 @@ void SPItem::set(SPAttributeEnum key, gchar const* value) {
             break;
         }
         case SP_PROP_CLIP_PATH: {
-            gchar *uri = extract_uri(value);
-            if (uri) {
+            auto uri = extract_uri(value);
+            if (!uri.empty()) {
                 try {
-                    item->clip_ref->attach(Inkscape::URI(uri));
+                    item->clip_ref->attach(Inkscape::URI(uri.c_str()));
                 } catch (Inkscape::BadURIException &e) {
                     g_warning("%s", e.what());
                     item->clip_ref->detach();
                 }
-                g_free(uri);
             } else {
                 item->clip_ref->detach();
             }
@@ -457,15 +456,14 @@ void SPItem::set(SPAttributeEnum key, gchar const* value) {
             break;
         }
         case SP_PROP_MASK: {
-            gchar *uri = extract_uri(value);
-            if (uri) {
+            auto uri = extract_uri(value);
+            if (!uri.empty()) {
                 try {
-                    item->mask_ref->attach(Inkscape::URI(uri));
+                    item->mask_ref->attach(Inkscape::URI(uri.c_str()));
                 } catch (Inkscape::BadURIException &e) {
                     g_warning("%s", e.what());
                     item->mask_ref->detach();
                 }
-                g_free(uri);
             } else {
                 item->mask_ref->detach();
             }
diff --git a/src/object/uri-references.cpp b/src/object/uri-references.cpp
index 3e11cd26c..69ed140e0 100644
--- a/src/object/uri-references.cpp
+++ b/src/object/uri-references.cpp
@@ -243,10 +243,9 @@ SPObject *sp_css_uri_reference_resolve(SPDocument *document, const gchar *uri)
     SPObject *ref = nullptr;
 
     if (document && uri && (strncmp(uri, "url(", 4) == 0)) {
-        gchar *trimmed = extract_uri(uri);
-        if (trimmed) {
-            ref = sp_uri_reference_resolve(document, trimmed);
-            g_free(trimmed);
+        auto trimmed = extract_uri(uri);
+        if (!trimmed.empty()) {
+            ref = sp_uri_reference_resolve(document, trimmed.c_str());
         }
     }
 
diff --git a/src/style-internal.cpp b/src/style-internal.cpp
index b02adec48..37371e18d 100644
--- a/src/style-internal.cpp
+++ b/src/style-internal.cpp
@@ -1245,8 +1245,8 @@ SPIPaint::read( gchar const *str ) {
         if ( strneq(str, "url", 3) ) {
 
             // FIXME: THE FOLLOWING CODE SHOULD BE PUT IN A PRIVATE FUNCTION FOR REUSE
-            gchar *uri = extract_uri( str, &str );
-            if(uri == nullptr || uri[0] == '\0') {
+            auto uri = extract_uri(str, &str);
+            if(uri.empty()) {
                 std::cerr << "SPIPaint::read: url is empty or invalid" << std::endl;
             } else if (!style ) {
                 std::cerr << "SPIPaint::read: url with empty SPStyle pointer" << std::endl;
@@ -1265,11 +1265,9 @@ SPIPaint::read( gchar const *str ) {
                     }
                 }
 
-                // std::cout << "uri: " << (uri?uri:"null") << std::endl;
                 // TODO check what this does in light of move away from union
-                sp_style_set_ipaint_to_uri_string ( style, this, uri);
+                sp_style_set_ipaint_to_uri_string(style, this, uri.c_str());
             }
-            g_free( uri );
         }
 
         while ( g_ascii_isspace(*str) ) {
@@ -1658,8 +1656,8 @@ SPIFilter::read( gchar const *str ) {
     } else if (streq(str, "none")) {
         set = true;
     } else if (strneq(str, "url", 3)) {
-        gchar *uri = extract_uri(str);
-        if (uri == nullptr || uri[0] == '\0') {
+        auto uri = extract_uri(str);
+        if (uri.empty()) {
             std::cerr << "SPIFilter::read: url is empty or invalid" << std::endl;
             return;
         } else if (!style) {
@@ -1684,12 +1682,11 @@ SPIFilter::read( gchar const *str ) {
 
         // We have href
         try {
-            href->attach(Inkscape::URI(uri));
+            href->attach(Inkscape::URI(uri.c_str()));
         } catch (Inkscape::BadURIException &e) {
             std::cerr << "SPIFilter::read() " << e.what() << std::endl;
             href->detach();
         }
-        g_free (uri);
 
     } else {
         std::cerr << "SPIFilter::read(): malformed value: " << str << std::endl;
diff --git a/testfiles/CMakeLists.txt b/testfiles/CMakeLists.txt
index d19d64dce..ddf3a66c4 100644
--- a/testfiles/CMakeLists.txt
+++ b/testfiles/CMakeLists.txt
@@ -15,6 +15,7 @@ include_directories("${CMAKE_SOURCE_DIR}/src/3rdparty/adaptagrams")
 
 set(TEST_SOURCES
 	uri-test
+	extract-uri-test
 	attributes-test
 	color-profile-test
 	dir-util-test
diff --git a/testfiles/src/cxxtests-to-migrate/extract-uri-test.h b/testfiles/src/cxxtests-to-migrate/extract-uri-test.h
deleted file mode 100644
index 189318cef..000000000
--- a/testfiles/src/cxxtests-to-migrate/extract-uri-test.h
+++ /dev/null
@@ -1,97 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/** @file
- * TODO: insert short description here
- *//*
- * Authors: see git history
- *
- * Copyright (C) 2016 Authors
- * Released under GNU GPL v2+, read the file 'COPYING' for more information.
- */
-
-#ifndef SEEN_EXTRACT_URI_TEST_H
-#define SEEN_EXTRACT_URI_TEST_H
-
-#include <cxxtest/TestSuite.h>
-
-#include "extract-uri.h"
-
-class ExtractURITest : public CxxTest::TestSuite
-{
-public:
-    void checkOne( char const* str, char const* expected )
-    {
-        gchar* result = extract_uri( str );
-        TS_ASSERT_EQUALS( ( result == NULL ), ( expected == NULL ) );
-        if ( result && expected ) {
-            TS_ASSERT_EQUALS( std::string(result), std::string(expected) );
-        } else if ( result ) {
-            TS_FAIL( std::string("Expected null, found (") + result + ")" );
-        } else if ( expected ) {
-            TS_FAIL( std::string("Expected (") + expected + "), found null" );
-        }
-        g_free( result );
-    }
-
-    void testBase()
-    {
-        char const* cases[][2] = {
-            { "url(#foo)", "#foo" },
-            { "url  foo  ", NULL },
-            { "url", NULL },
-            { "url ", NULL },
-            { "url()", NULL },
-            { "url ( ) ", NULL },
-            { "url foo bar ", NULL },
-        };
-
-        for ( size_t i = 0; i < G_N_ELEMENTS(cases); i++ )
-        {
-            checkOne( cases[i][0], cases[i][1] );
-        }
-    }
-
-    void testWithTrailing()
-    {
-        char const* cases[][2] = {
-            { "url(#foo) bar", "#foo" },
-            { "url() bar", NULL },
-            { "url ( ) bar ", NULL }
-        };
-
-        for ( size_t i = 0; i < G_N_ELEMENTS(cases); i++ )
-        {
-            checkOne( cases[i][0], cases[i][1] );
-        }
-    }
-
-    void testQuoted()
-    {
-        char const* cases[][2] = {
-            { "url('#foo')", "#foo" },
-            { "url(\"#foo\")", "#foo" },
-            { "url('#f o o')", "#f o o" },
-            { "url(\"#f o o\")", "#f o o" },
-            { "url('#fo\"o')", "#fo\"o" },
-            { "url(\"#fo'o\")", "#fo'o" },
-        };
-
-        for ( size_t i = 0; i < G_N_ELEMENTS(cases); i++ )
-        {
-            checkOne( cases[i][0], cases[i][1] );
-        }
-    }
-
-};
-
-#endif // SEEN_EXTRACT_URI_TEST_H
-
-/*
-  Local Variables:
-  mode:c++
-  c-file-style:"stroustrup"
-  c-file-offsets:((innamespace . 0)(inline-open . 0)(case-label . +))
-  indent-tabs-mode:nil
-  fill-column:99
-  End:
-*/
-// vim: filetype=cpp:expandtab:shiftwidth=4:tabstop=8:softtabstop=4:fileencoding=utf-8:textwidth=99 :
diff --git a/testfiles/src/extract-uri-test.cpp b/testfiles/src/extract-uri-test.cpp
new file mode 100644
index 000000000..acff9669a
--- /dev/null
+++ b/testfiles/src/extract-uri-test.cpp
@@ -0,0 +1,75 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/**
+ * @file
+ * Test extract_uri
+ */
+/*
+ * Authors:
+ *   Thomas Holder
+ *
+ * Copyright (C) 2018 Authors
+ *
+ * Released under GNU GPL v2+, read the file 'COPYING' for more information.
+ */
+
+#include "extract-uri.h"
+#include "gtest/gtest.h"
+
+TEST(ExtractUriTest, valid)
+{
+    ASSERT_EQ(extract_uri("url(#foo)"), "#foo");
+    ASSERT_EQ(extract_uri("url( \t #foo \t )"), "#foo");
+    ASSERT_EQ(extract_uri("url( '#foo' )"), "#foo");
+    ASSERT_EQ(extract_uri("url('url(foo)')"), "url(foo)");
+    ASSERT_EQ(extract_uri("url(\"foo(url)\")"), "foo(url)");
+    ASSERT_EQ(extract_uri("url()bar"), "");
+    ASSERT_EQ(extract_uri("url( )bar"), "");
+    ASSERT_EQ(extract_uri("url(a b)"), "a b");
+}
+
+TEST(ExtractUriTest, legacy)
+{
+    ASSERT_EQ(extract_uri("url (foo)"), "foo");
+}
+
+TEST(ExtractUriTest, invalid)
+{
+    ASSERT_EQ(extract_uri("#foo"), "");
+    ASSERT_EQ(extract_uri(" url(foo)"), "");
+    ASSERT_EQ(extract_uri("url(#foo"), "");
+    ASSERT_EQ(extract_uri("url('#foo'"), "");
+    ASSERT_EQ(extract_uri("url('#foo)"), "");
+    ASSERT_EQ(extract_uri("url #foo)"), "");
+}
+
+static char const *extract_end(char const *s)
+{
+    char const *end = nullptr;
+    extract_uri(s, &end);
+    return end;
+}
+
+TEST(ExtractUriTest, endptr)
+{
+    ASSERT_STREQ(extract_end(""), nullptr);
+    ASSERT_STREQ(extract_end("url(invalid"), nullptr);
+    ASSERT_STREQ(extract_end("url('invalid)"), nullptr);
+    ASSERT_STREQ(extract_end("url(valid)"), "");
+    ASSERT_STREQ(extract_end("url(valid)foo"), "foo");
+    ASSERT_STREQ(extract_end("url('valid')bar"), "bar");
+    ASSERT_STREQ(extract_end("url(  'valid'  )bar"), "bar");
+    ASSERT_STREQ(extract_end("url(  valid  ) bar "), " bar ");
+    ASSERT_STREQ(extract_end("url()bar"), "bar");
+    ASSERT_STREQ(extract_end("url( )bar"), "bar");
+}
+
+/*
+  Local Variables:
+  mode:c++
+  c-file-style:"stroustrup"
+  c-file-offsets:((innamespace . 0)(inline-open . 0)(case-label . +))
+  indent-tabs-mode:nil
+  fill-column:99
+  End:
+*/
+// vim: filetype=cpp:expandtab:shiftwidth=4:tabstop=8:softtabstop=4:fileencoding=utf-8:textwidth=99 :