GNOME Bugzilla – Bug 678549
Add some NULL-safe compare APIs
Last modified: 2018-05-24 14:17:27 UTC
g_str_equal0, g_variant_equal0
Created attachment 216935 [details] [review] Add (and test) g_str_{hash,equal}0. Also, modify the documentation for g_str_equal() to remove the suggestion that g_strcmp0() is a better choice. g_str_equal() is vastly better in terms of readability and avoids the common logic problems associated with use of strcmp-class functions.
Created attachment 216936 [details] [review] Add (and test) g_variant_equal0().
This could use a bit of motivation; is there any existing code in GLib or GNOME that wants to use this?
lots of people use (g_strcmp0() == 0) in their conditions, so g_str_equal0() would at least improve readability; plus, I don't think changing the preconditions on g_str_equal() to accept NULL is a good idea either, so g_str_equal0() would also catch the (str != NULL && g_str_equal (str, "foo")) pattern.
(In reply to comment #4) > lots of people use (g_strcmp0() == 0) in their conditions, $ pwd /src/glib $ git grep 'g_strcmp0.*== 0' | wc -l 210 $ Looks right. > so g_str_equal0() > would at least improve readability; plus, I don't think changing the > preconditions on g_str_equal() to accept NULL is a good idea either, Agree with that. so > g_str_equal0() would also catch the (str != NULL && g_str_equal (str, "foo")) > pattern. $ git grep '!= NULL &&.* g_str_equal' | wc -l 0 $ Anyways, my question was just about ensuring that we're trying to, for new API proposals, ensuring everyone involved in the discussion has a good idea of where and how exactly the new API would be used.
On the GVariant side: David requested this one, and I've also found myself wanting it a lot.
(In reply to comment #4) > I don't think changing the > preconditions on g_str_equal() to accept NULL is a good idea either Why? We had no choice but to make a new function for g_strcmp0(), but g_str_equal() will currently crash if either argument is NULL, so extending it to support NULL couldn't break any currently-working code.
I prefer that g_str_equal() crashes. The fact that it does has allowed me to very quickly find bugs. In fact, the thing that led me to working on this patch in the first place was that I found NULL creeping in somewhere unexpected and had to change some other (related) code to check for it.
(In reply to comment #6) > On the GVariant side: David requested this one, and I've also found myself > wanting it a lot. URLs/git repository names? (I know I'm being painful about this...sorry)
We use this pattern heavily in Builder. In fact, we have two that are quite useful: ide_str_equal0 ide_str_empty0 I'd love to see them in glib directly. Any chance we can push this through?
My position on this should be well known: NULL is not a string I much prefer to have an explicit if (str == NULL) case in the code before any use of string APIs. Mixing NULL and "" in the api is only going to increase the confusion of programmers who are already confused about it.
(In reply to Matthias Clasen from comment #11) > My position on this should be well known: NULL is not a string > > I much prefer to have an explicit if (str == NULL) case in the code before > any use of string APIs. > > Mixing NULL and "" in the api is only going to increase the confusion of > programmers who are already confused about it. For prior art, C# has String.IsNullOrEmpty() which I think still makes it clear that they are separate things. I'm not suggesting that "g_str_equal0()" would equate NULL and "". What I find it convenient for is string setters for the same reasons as g_set_object(). It helps fight excessive notify emissions. if (!ide_str_equal0 (priv->a, a)) { g_free (priv->a); priv->a = g_strdup (a); g_object_notify (self, "a"); } Now, if we had a more abstract way to write string properties, I do imagine most of my usage of this would disappear (but not all).
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/558.