From a9ee3a5faadeea24762c64d98eb10145906e014e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 17 Jan 2023 17:49:44 +0100 Subject: [PATCH 1/5] gstrfuncs: Ignore parsing inline macro versions for docs and g-i We have actual definitions for these functions so we should ignore their inline versions, not to potentially break doc parsers due to the different argument names. For some reasons we can't merge the check together with the gnu C check if, otherwise the check gets ignored by doc parser. --- glib/gstrfuncs.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/glib/gstrfuncs.h b/glib/gstrfuncs.h index 93a819305..56d78cf87 100644 --- a/glib/gstrfuncs.h +++ b/glib/gstrfuncs.h @@ -147,6 +147,7 @@ gboolean (g_str_has_prefix) (const gchar *str, const gchar *prefix); #if G_GNUC_CHECK_VERSION (2, 0) +#if !defined (__GTK_DOC_IGNORE__) && !defined (__GI_SCANNER__) /* This macro is defeat a false -Wnonnull warning in GCC. * Without it, it thinks strlen and memcmp may be getting passed NULL @@ -202,6 +203,7 @@ gboolean (g_str_has_prefix) (const gchar *str, (g_str_has_suffix) (STR, SUFFIX) \ ) +#endif /* !defined (__GTK_DOC_IGNORE__) && !defined (__GI_SCANNER__) */ #endif /* G_GNUC_CHECK_VERSION (2, 0) */ /* String to/from double conversion functions */ From 9cd90d97ae16fe32caa13b92ce58f1b295a0b7f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 17 Jan 2023 18:42:11 +0100 Subject: [PATCH 2/5] tests/strfuncs: Ensure that inlined functions really are macros Add a compile check to ensure that the functions we have inlined are actually macros when under gcc (and friends). --- glib/tests/strfuncs.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/glib/tests/strfuncs.c b/glib/tests/strfuncs.c index 3c96eec15..bd10778e1 100644 --- a/glib/tests/strfuncs.c +++ b/glib/tests/strfuncs.c @@ -1238,6 +1238,14 @@ test_has_prefix (void) static void test_has_prefix_macro (void) { + #if G_GNUC_CHECK_VERSION (2, 0) + #ifndef g_str_has_prefix + #error g_str_has_prefix() should be defined as a macro in this platform! + #endif + #else + g_test_incomplete ("g_str_has_prefix() is not inlined in this platform"); + #endif + if (g_test_undefined ()) { g_test_expect_message (G_LOG_DOMAIN, G_LOG_LEVEL_CRITICAL, @@ -1305,6 +1313,14 @@ test_has_suffix (void) static void test_has_suffix_macro (void) { + #if G_GNUC_CHECK_VERSION (2, 0) + #ifndef g_str_has_suffix + #error g_str_has_suffix() should be defined as a macro in this platform! + #endif + #else + g_test_incomplete ("g_str_has_suffix() is not inlined in this platform"); + #endif + if (g_test_undefined ()) { g_test_expect_message (G_LOG_DOMAIN, G_LOG_LEVEL_CRITICAL, From ed42f5770419223c7094aa17db290ae6d22c8c31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 17 Jan 2023 16:25:46 +0100 Subject: [PATCH 3/5] tests/strfuncs: Add test for g_strdup() with empty string --- glib/tests/strfuncs.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/glib/tests/strfuncs.c b/glib/tests/strfuncs.c index bd10778e1..4551f1c82 100644 --- a/glib/tests/strfuncs.c +++ b/glib/tests/strfuncs.c @@ -505,6 +505,11 @@ test_strdup (void) g_assert_nonnull (str); g_assert_cmpstr (str, ==, GLIB_TEST_STRING); g_free (str); + + str = g_strdup (""); + g_assert_cmpint (str[0], ==, '\0'); + g_assert_cmpstr (str, ==, ""); + g_clear_pointer (&str, g_free); } /* Testing g_strndup() function with various positive and negative cases */ From cc0fb5e77c7a8c10951aef2b539aeec1ba0d9ff0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 17 Jan 2023 16:37:28 +0100 Subject: [PATCH 4/5] gstrfuncs: Add inline version of g_strdup() g_strdup() is often used to duplicate static strings, in these cases the compiler could use a faster path because it knows the length of the string at compile time, but this cannot happen because our g_strdup() implementation is hidden. To improve this case, we add a simple implementation of g_strdup() when it is used with static or NULL strings that explicitly uses strlen, g_malloc and memcpy to give hints to the compiler how to behave better. This has definitely some benefits in terms of performances, causing an iteration of 1000000 string duplication to drop from 2.7002s to 1.9428s for a static string and from ~0.6584s to ~0.4408 for a NULL one. Since compiler can optimize these cases quite a bit, the generated code [2] is not increasing a lot, given that it can now avoid generating some code or do it in few simpler steps. Update tests to cover both inlined and non inlined cases. [1] https://gitlab.gnome.org/GNOME/glib/-/merge_requests/3209#note_1644383 [2] https://gitlab.gnome.org/GNOME/glib/-/merge_requests/3209#note_1646662 --- glib/gstrfuncs.c | 4 ++-- glib/gstrfuncs.h | 21 +++++++++++++++++++-- glib/tests/cxx.cpp | 29 ++++++++++++++++++++++++++++ glib/tests/strfuncs.c | 44 ++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 93 insertions(+), 5 deletions(-) diff --git a/glib/gstrfuncs.c b/glib/gstrfuncs.c index 8525e4692..665b2071b 100644 --- a/glib/gstrfuncs.c +++ b/glib/gstrfuncs.c @@ -352,12 +352,12 @@ get_C_locale (void) * Returns: a newly-allocated copy of @str */ gchar* -g_strdup (const gchar *str) +(g_strdup) (const gchar *str) { gchar *new_str; gsize length; - if (str) + if G_LIKELY (str) { length = strlen (str) + 1; new_str = g_new (char, length); diff --git a/glib/gstrfuncs.h b/glib/gstrfuncs.h index 56d78cf87..569f96977 100644 --- a/glib/gstrfuncs.h +++ b/glib/gstrfuncs.h @@ -153,7 +153,7 @@ gboolean (g_str_has_prefix) (const gchar *str, * Without it, it thinks strlen and memcmp may be getting passed NULL * despite the explicit check for NULL right above the calls. */ -#define _G_STR_NONNULL(x) (x + !x) +#define _G_STR_NONNULL(x) ((x) + !(x)) #define g_str_has_prefix(STR, PREFIX) \ (__builtin_constant_p (PREFIX)? \ @@ -203,6 +203,23 @@ gboolean (g_str_has_prefix) (const gchar *str, (g_str_has_suffix) (STR, SUFFIX) \ ) +#define g_strdup(STR) \ + (__builtin_constant_p ((STR)) ? \ + (G_LIKELY ((STR) != NULL) ? \ + G_GNUC_EXTENSION ({ \ + const char *const ___str = ((STR)); \ + const char *const __str = _G_STR_NONNULL (___str); \ + const size_t __str_len = strlen (__str) + 1; \ + char *__dup_str = (char *) g_malloc (__str_len); \ + (char *) memcpy (__dup_str, __str, __str_len); \ + }) \ + : \ + (char *) (NULL) \ + ) \ + : \ + (g_strdup) ((STR)) \ + ) + #endif /* !defined (__GTK_DOC_IGNORE__) && !defined (__GI_SCANNER__) */ #endif /* G_GNUC_CHECK_VERSION (2, 0) */ @@ -279,7 +296,7 @@ gchar* g_strup (gchar *string); * ought to be freed with g_free from the caller at some point. */ GLIB_AVAILABLE_IN_ALL -gchar* g_strdup (const gchar *str) G_GNUC_MALLOC; +gchar* (g_strdup) (const gchar *str) G_GNUC_MALLOC; GLIB_AVAILABLE_IN_ALL gchar* g_strdup_printf (const gchar *format, ...) G_GNUC_PRINTF (1, 2) G_GNUC_MALLOC; diff --git a/glib/tests/cxx.cpp b/glib/tests/cxx.cpp index 6e4727f2a..8616a18db 100644 --- a/glib/tests/cxx.cpp +++ b/glib/tests/cxx.cpp @@ -304,6 +304,7 @@ test_str_equal (void) { const char *str_a = "a"; char *str_b = g_strdup ("b"); + char *str_null = g_strdup (NULL); gconstpointer str_a_ptr = str_a, str_b_ptr = str_b; const unsigned char *str_c = (const unsigned char *) "c"; @@ -317,10 +318,36 @@ test_str_equal (void) g_assert_true (g_str_equal (str_a, str_a_ptr)); g_assert_false (g_str_equal (str_a_ptr, str_b_ptr)); g_assert_false (g_str_equal (str_c, str_b)); + g_assert_cmpstr (str_b, !=, str_null); g_free (str_b); } +static void +test_strdup (void) +{ + gchar *str; + + g_assert_null ((g_strdup) (NULL)); + + str = (g_strdup) ("C++ is cool too!"); + g_assert_nonnull (str); + g_assert_cmpstr (str, ==, "C++ is cool too!"); + g_free (str); +} + +static void +test_strdup_macro (void) +{ + gchar *str; + + g_assert_null (g_strdup (NULL)); + + str = g_strdup ("C++ is cool too!"); + g_assert_nonnull (str); + g_assert_cmpstr (str, ==, "C++ is cool too!"); + g_free (str); +} static void test_string_append (void) @@ -444,6 +471,8 @@ main (int argc, char *argv[]) g_test_add_func ("/C++/clear-pointer", test_clear_pointer); g_test_add_func ("/C++/steal-pointer", test_steal_pointer); g_test_add_func ("/C++/str-equal", test_str_equal); + g_test_add_func ("/C++/strdup", test_strdup); + g_test_add_func ("/C++/strdup/macro", test_strdup_macro); g_test_add_func ("/C++/string-append", test_string_append); return g_test_run (); diff --git a/glib/tests/strfuncs.c b/glib/tests/strfuncs.c index 4551f1c82..96f378815 100644 --- a/glib/tests/strfuncs.c +++ b/glib/tests/strfuncs.c @@ -499,12 +499,53 @@ test_strdup (void) { gchar *str; + g_assert_null ((g_strdup) (NULL)); + + str = (g_strdup) (GLIB_TEST_STRING); + g_assert_nonnull (str); + g_assert_cmpstr (str, ==, GLIB_TEST_STRING); + + char *other_str = (g_strdup) (str); + g_free (str); + + g_assert_nonnull (other_str); + g_assert_cmpstr (other_str, ==, GLIB_TEST_STRING); + g_clear_pointer (&other_str, g_free); + + str = (g_strdup) (""); + g_assert_cmpint (str[0], ==, '\0'); + g_assert_cmpstr (str, ==, ""); + g_clear_pointer (&str, g_free); +} + +static void +test_strdup_inline (void) +{ + gchar *str; + + #if G_GNUC_CHECK_VERSION (2, 0) + #ifndef g_strdup + #error g_strdup() should be defined as a macro in this platform! + #endif + #else + g_test_incomplete ("g_strdup() is not inlined in this platform"); + #endif + + /* Testing inline version of g_strdup() function with various positive and + * negative cases */ + g_assert_null (g_strdup (NULL)); str = g_strdup (GLIB_TEST_STRING); g_assert_nonnull (str); g_assert_cmpstr (str, ==, GLIB_TEST_STRING); - g_free (str); + + char *other_str = g_strdup (str); + g_clear_pointer (&str, g_free); + + g_assert_nonnull (other_str); + g_assert_cmpstr (other_str, ==, GLIB_TEST_STRING); + g_clear_pointer (&other_str, g_free); str = g_strdup (""); g_assert_cmpint (str[0], ==, '\0'); @@ -2698,6 +2739,7 @@ main (int argc, g_test_add_func ("/strfuncs/strconcat", test_strconcat); g_test_add_func ("/strfuncs/strdelimit", test_strdelimit); g_test_add_func ("/strfuncs/strdup", test_strdup); + g_test_add_func ("/strfuncs/strdup/inline", test_strdup_inline); g_test_add_func ("/strfuncs/strdup-printf", test_strdup_printf); g_test_add_func ("/strfuncs/strdupv", test_strdupv); g_test_add_func ("/strfuncs/strerror", test_strerror); From 23da6bade053463a823bbfc4520b71a4ade2b84b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 25 Jan 2023 16:06:06 +0100 Subject: [PATCH 5/5] {glib,gio}/cxx: Add more tests for C++ inline funcs These could behave differently in C++ so let's ensure this is not the case. --- gio/tests/cxx.cpp | 45 ++++++++++++++++++++++++++++++++++++++++++--- glib/tests/cxx.cpp | 28 ++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 3 deletions(-) diff --git a/gio/tests/cxx.cpp b/gio/tests/cxx.cpp index 60d295b19..4af7b3b5f 100644 --- a/gio/tests/cxx.cpp +++ b/gio/tests/cxx.cpp @@ -21,8 +21,47 @@ #include -int -main () +static void +test_name (void) { - return 0; + GTask *task = NULL; + char *orig = g_strdup ("some task"); + + task = g_task_new (NULL, NULL, NULL, NULL); + (g_task_set_name) (task, orig); + g_assert_cmpstr (g_task_get_name (task), ==, "some task"); + + (g_task_set_name) (task, "some other name"); + g_assert_cmpstr (g_task_get_name (task), ==, "some other name"); + + g_clear_object (&task); + g_free (orig); +} + +static void +test_name_macro_wrapper (void) +{ + GTask *task = NULL; + char *orig = g_strdup ("some task"); + + task = g_task_new (NULL, NULL, NULL, NULL); + g_task_set_name (task, orig); + g_assert_cmpstr (g_task_get_name (task), ==, "some task"); + + g_task_set_name (task, "some other name"); + g_assert_cmpstr (g_task_get_name (task), ==, "some other name"); + + g_clear_object (&task); + g_free (orig); +} + +int +main (int argc, char **argv) +{ + g_test_init (&argc, &argv, NULL); + + g_test_add_func ("/gtask/name", test_name); + g_test_add_func ("/gtask/name/macro-wrapper", test_name_macro_wrapper); + + return g_test_run (); } diff --git a/glib/tests/cxx.cpp b/glib/tests/cxx.cpp index 8616a18db..abd8a8d67 100644 --- a/glib/tests/cxx.cpp +++ b/glib/tests/cxx.cpp @@ -349,6 +349,30 @@ test_strdup_macro (void) g_free (str); } +static void +test_str_has_prefix (void) +{ + g_assert_true ((g_str_has_prefix) ("C++ is cool!", "C++")); +} + +static void +test_str_has_prefix_macro (void) +{ + g_assert_true (g_str_has_prefix ("C++ is cool!", "C++")); +} + +static void +test_str_has_suffix (void) +{ + g_assert_true ((g_str_has_suffix) ("C++ is cool!", "cool!")); +} + +static void +test_str_has_suffix_macro (void) +{ + g_assert_true (g_str_has_suffix ("C++ is cool!", "cool!")); +} + static void test_string_append (void) { @@ -473,6 +497,10 @@ main (int argc, char *argv[]) g_test_add_func ("/C++/str-equal", test_str_equal); g_test_add_func ("/C++/strdup", test_strdup); g_test_add_func ("/C++/strdup/macro", test_strdup_macro); + g_test_add_func ("/C++/str-has-prefix", test_str_has_prefix); + g_test_add_func ("/C++/str-has-prefix/macro", test_str_has_prefix_macro); + g_test_add_func ("/C++/str-has-suffix", test_str_has_suffix); + g_test_add_func ("/C++/str-has-suffix/macro", test_str_has_suffix_macro); g_test_add_func ("/C++/string-append", test_string_append); return g_test_run ();