GNOME Bugzilla – Bug 685880
Add g_strv_contains()
Last modified: 2016-05-24 13:02:41 UTC
In telepathy-glib we have tp_strv_contains(), and I think lot of projects just reimplement it: gboolean tp_strv_contains (const gchar * const *strv, const gchar *str) { g_return_val_if_fail (str != NULL, FALSE); if (strv == NULL) return FALSE; while (*strv != NULL) { if (!tp_strdiff (str, *strv)) return TRUE; strv++; } return FALSE; } the tp_strdiff() call can trivially be replaced by g_strcmp0(). See https://bugzilla.gnome.org/show_bug.cgi?id=685878.
Created attachment 241574 [details] [review] Add g_strv_contains() Copied from telepathy-glib's tp_strv_contains().
OK, you're proposing a lot of new API. There's a basic guideline: link to at least two modules that would use it.
Review of attachment 241574 [details] [review]: Under what situations does one want to search for an individual string in a strv? Wouldn't applications be better suited using a GHashTable?
(In reply to comment #3) > Review of attachment 241574 [details] [review]: > > Under what situations does one want to search for an individual string in a > strv? Wouldn't applications be better suited using a GHashTable? For small strv I don't think GHashTable would help. GStrv is also what GVariant gives you for "as" dbus type. (In reply to comment #2) > OK, you're proposing a lot of new API. There's a basic guideline: link to at > least two modules that would use it. find -name "*.c" | xargs grep strv_contains | wc -l 191 Most are telepathy components, but I see some in gnome-control-center as well. And it does not count all modules that just reimplemented it, or used another name.
(In reply to comment #2) > OK, you're proposing a lot of new API. There's a basic guideline: link to at > least two modules that would use it. I was just looking for this function when working on a patch: https://bugzilla.gnome.org/review?bug=697363&attachment=241242 (pygi-info.c) But I'm happy to use GHashTable here as well if you think it is a better solution in this case.
g_strdiff()? fwiw, I've wanted this functionality on a few occasions. Why the (skip) and where's the docs?
(In reply to comment #6) > g_strdiff()? > > fwiw, I've wanted this functionality on a few occasions. Why the (skip) and > where's the docs? bug #685878
Setting depends on bug #685878, even if it can trivially be implemented with g_strcmp0() instead of g_strdiff.
Skip annotation is because language bindings probably already have their string comparison built-in, so I don't think you'll ever use glib's.
(In reply to comment #9) > Skip annotation is because language bindings probably already have their string > comparison built-in, so I don't think you'll ever use glib's. and for an strv as well, in python you would do if "foo" in strv: and not if strv.contains("foo"): I guess. Right?
In Python strvs are automatically converted to and from lists of strings. So it doesn't make sense for Python at least, not sure if other languages would benefit from it though.
I would love to have g_strv_contains() as well. Please?
Created attachment 267685 [details] [review] Add g_strv_contains()
g_strdiff() was actually useless in this case since both strings are always != NULL. Any objection on this patch ?
Created attachment 273888 [details] [review] Add g_strv_contains() Same patch, removing the special-casing of %NULL and the (skip).
Review of attachment 273888 [details] [review]: ::: glib/gstrfuncs.c @@ +3090,3 @@ +gboolean +g_strv_contains (const gchar * const *strv, + const gchar *str) Whitespace @@ +3092,3 @@ + const gchar *str) +{ + g_return_val_if_fail (str != NULL, FALSE); We should probably also add a precondition for @strv ::: glib/gstrfuncs.h @@ +302,3 @@ gboolean accept_alternates); +GLIB_AVAILABLE_IN_2_40 2_42
Created attachment 291448 [details] [review] gstrfuncs: Add g_strv_contains() Includes unit tests.
(In reply to comment #17) > Created an attachment (id=291448) [details] [review] > gstrfuncs: Add g_strv_contains() > > Includes unit tests. This fixes Lars’ points from comment #16, clarifies the nullability of @strv in the docs (since various g_strv*() functions handle NULL GStrvs differently), and changes from a while-loop to a for-loop for clarity. It also adds some unit tests.
Review of attachment 291448 [details] [review]: Thanks!
I don't see why comment 15 removed the handling of NULL strv. IMHO it's quite useful to handle NULL strv too, and the result if obviously FALSE for any string.
(In reply to comment #20) > I don't see why comment 15 removed the handling of NULL strv. IMHO it's quite > useful to handle NULL strv too, and the result if obviously FALSE for any > string. NULL is not a valid strv. Passing NULL when a strv is expected should result in a critical. We don't treat NULL as empty string either in functions accepting strings. Other functions handlings strvs also don't allow NULL (e.g., g_strv_length and g_strjoinv). The only exceptions are g_strfreev (functions freeing stuff always accept NULL) and g_strdupv (to mirror g_strdup).
Comment on attachment 291448 [details] [review] gstrfuncs: Add g_strv_contains() Committed; thanks for the fast turnaround! commit 71944b1bfd2cff57e889b806d001458dce6fa2b5 Author: Xavier Claessens <xavier.claessens@collabora.co.uk> Date: Mon Apr 15 14:54:31 2013 +0200 gstrfuncs: Add g_strv_contains() Includes unit tests. https://bugzilla.gnome.org/show_bug.cgi?id=685880 docs/reference/glib/glib-sections.txt | 1 + glib/gstrfuncs.c | 27 +++++++++++++++++++++++++++ glib/gstrfuncs.h | 4 ++++ glib/tests/strfuncs.c | 19 +++++++++++++++++++ 4 files changed, 51 insertions(+)
Can't pass GStrv without ugly casting? #include <glib.h> int main(int argc, char *argv[]) { GStrv foo = NULL; g_strv_contains(foo, "blah"); return 0; } main.c: In function ‘main’: main.c:5:18: warning: passing argument 1 of ‘g_strv_contains’ from incompatible pointer type [-Wincompatible-pointer-types] g_strv_contains(foo, "blah"); ^~~ In file included from /usr/include/glib-2.0/glib.h:79:0, from main.c:1: /usr/include/glib-2.0/glib/gstrfuncs.h:306:23: note: expected ‘const gchar * const* {aka const char * const*}’ but argument is of type ‘GStrv {aka char **}’ gboolean g_strv_contains (const gchar * const *strv, ^~~~~~~~~~~~~~~
gcc doesn't want to cast "gchar**" to "const gchar * const *", it has always annoyed me. People often wrongly use "const gchar **" instead to avoid that warning. I'm sure there are really good reasons for this deep inside the C grammar...