After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 399880 - Add macro for checking if strings are NULL or empty
Add macro for checking if strings are NULL or empty
Status: RESOLVED WONTFIX
Product: glib
Classification: Platform
Component: general
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: gtkdev
gtkdev
: 629215 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2007-01-23 18:13 UTC by Martyn Russell
Modified: 2013-10-24 13:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch to fix this bug (1.60 KB, patch)
2007-01-23 18:14 UTC, Martyn Russell
none Details | Review
Add g_str_empty() (1.75 KB, patch)
2013-04-15 15:14 UTC, Xavier Claessens
none Details | Review

Description Martyn Russell 2007-01-23 18:13:28 UTC
I have seen this sort of macro defined commonly by people in their own projects from time to time and thought it might make sense to add such a thing to glib.

Patch to follow.
Comment 1 Martyn Russell 2007-01-23 18:14:08 UTC
Created attachment 81004 [details] [review]
Patch to fix this bug
Comment 2 Owen Taylor 2008-01-02 14:46:37 UTC
Much clearer if just inlined - I would expect G_STR_EMPTY to be *str == '\0',
not this.
Comment 3 Martyn Russell 2008-01-02 15:03:10 UTC
Hmm, but if G_STR_EMPTY was "*str == '\0'" and you passed a NULL pointer, surely it would crash? i.e. you would have to check str before dereferencing. The whole point about this is it does those 2 steps with one call to a macro.

The benefit of this macro is really noticed when you have to check multiple strings.

For example, consider:

  if ((!foo || *foo == '\0') &&
      (!my_text || *my_text == '\0') &&
      (!some_long_string || *some_long_string == '\0')) {

And:

  if (G_STR_EMPTY (foo) &&
      G_STR_EMPTY (my_text) &&
      G_STR_EMPTY (some_long_string)) {

The later is much quicker to read and maintainable IMO. You might argue that the developer would have to know what the macro did, but if they used it anything like as much as I do in my applications, they would anyway.
Comment 4 Owen Taylor 2008-01-02 16:28:08 UTC
If you have a bunch of places where you have either empty strings
or null strings then, well, in my opinion (and only my opinion)
you need to reconsider how you think about strings!

 "" - a string with no characters
 NULL - not a string

If there is a legitimate reason why you need to special case those
two (entirely different!) values the same way ... say, you are 
converting empty strings to NULL to save allocations ... then a
temporary local #define is an easy and thing to do. But it should
not be enshrined into GLib.
Comment 5 Mathias Hasselmann (IRC: tbf) 2008-01-02 17:24:24 UTC
Owen: I don't know why Martyn frequently hits this cases, and web programming definitly isn't glib's primary target. But (in Java) I had to use Martyn's idiom quite often when processing Web requests:

  String blub = request.getParameter ("whatever");

  if (null != blub && blub.length() > 0) {
      ...
  }

Every half-serious Java framework provides some static isEmpty() method which operates in Martyn's spirit.

Guess you also hit that pattern quite often, when processing XML or configuration files.

In summary: When dealing with user provided data you quite often have situations were you need Martyn's idiom. Of course you could hack helper functions of the kind:

  gchar* maman_get_value (..., const gchar *preset)
  {
    gchar *value = maman_real_get_value (...);

    if (NULL == value || !*value)
      value = g_strdup (preset);

    return value;
  }

But why do that stunt, when a simple macro as suggested by Martyn does the same job with less effort?
Comment 6 Xavier Claessens 2008-01-02 17:34:09 UTC
I personaly use that macro in 2 cases:

1) DBus always returns "" as an empty string, in that case NULL is the same than "". I could just check if *str=='\0' but I prefer that macro because it makes sure str is not NULL so it won't crash my application.

2) Things like gtk_entry_get_text() often (always?) returns "" for empty string. And like for the dbus case, I prefer the first check if it's NULL before deferencing.

In many cases applications handle "" and NULL exactly the same way, it's just an empty string.
Comment 7 Owen Taylor 2008-01-02 17:35:28 UTC
Other than g_free (which doesn't really take strings) there are very few
or no functions in GLib which take either a string or NULL (and I'm very
opposed to confusion between empty strings and NULL), so the function
would need to be named G_STR_IS_NULL_OR_EMPTY() but at that point, it
doesn't seem to be a very useful addition to me.


[

 The check I do often want, is

 a == b || (a != NULL && b != NULL && strcmp(a, b) == 0))

]
Comment 8 Owen Taylor 2008-01-02 20:16:14 UTC
In terms of D-BUS. It has strings. Strings maybe be empty .. that is,
they may be "". There is no such thing as a null value for a string
in D-BUS.
Comment 9 Martyn Russell 2008-01-03 01:11:10 UTC
(In reply to comment #4)
> If you have a bunch of places where you have either empty strings
> or null strings then, well, in my opinion (and only my opinion)
> you need to reconsider how you think about strings!
> 
>  "" - a string with no characters
>  NULL - not a string

:)

I don't think of them as the same thing. I merely have a common use for it across many applications and have seen this macro reimplemented time and time again to do the exact same thing.
 
(In reply to comment #7)
> Other than g_free (which doesn't really take strings)

Not sure I understand what you mean here? If you mean string as in "gchar *", then g_free is used everywhere to free my strings.

> (and I'm very opposed to confusion between empty strings and NULL), 

I can understand not wanting the confusion, but in essence, when writing applications and serialising/deserialising your data, you are often only interested in if a string has any real data in it or not. This function is deal for that.

> so the function
> would need to be named G_STR_IS_NULL_OR_EMPTY() but at that point, it
> doesn't seem to be a very useful addition to me.

I agree. But really, some documentation describing this would be sufficient.
 
> [
> 
>  The check I do often want, is
> 
>  a == b || (a != NULL && b != NULL && strcmp(a, b) == 0))
> 
> ]

I too often want this, but less so than the current issue.

You will have to forgive the misuse of name spacing in Gossip :) but I use this macro a lot:

  $  grep -r --include='*.[ch]' G_STR_EMPTY ./ | wc -l
  104

> In terms of D-BUS. It has strings. Strings maybe be empty .. that is,
> they may be "". There is no such thing as a null value for a string
> in D-BUS.

True, but it is protective programming to check for NULL before dereferencing and I tend to favour that too.
Comment 10 Vincent Untz 2008-04-26 09:51:55 UTC
Here's another voice supporting this macro. Can we reopen this bug?
Comment 11 Christian Persch 2010-09-10 09:26:30 UTC
*** Bug 629215 has been marked as a duplicate of this bug. ***
Comment 12 Martyn Russell 2010-09-10 11:37:48 UTC
(In reply to comment #5)
> Owen: I don't know why Martyn frequently hits this cases, and web programming
> definitly isn't glib's primary target. But (in Java) I had to use Martyn's
> idiom quite often when processing Web requests:

Not sure why you think I am doing "web programming" here btw.
 
> Every half-serious Java framework provides some static isEmpty() method which
> operates in Martyn's spirit.

Yep.
 
(In reply to comment #10)
> Here's another voice supporting this macro. Can we reopen this bug?

Yes please, can we reopen this bug :)

Matthias, do you have an opinion here?
Comment 13 Matthias Clasen 2010-09-10 11:49:25 UTC
I don't think 'half-decent Java frameworks' have much bearing on what glib should or shouldn't provide. I am with Owen in believing that naked C expressions like

if (*s == '\0') 

or 

if (s == NULL || *s == '\0')

are nicer when dealing with C strings. 

Not sure where this desire to wrap everything in a macro comes. It only makes you look up the macro definition every time to double-check whether it accepts NULL or not. 

If you really need this macro, you can easily add it to your g-things-glib-maintainers-stubbornly-refused.h header.
Comment 14 Martyn Russell 2010-09-10 13:15:06 UTC
(In reply to comment #13)
> I don't think 'half-decent Java frameworks' have much bearing on what glib
> should or shouldn't provide.

The point here is, it is done in other languages because they recognise a common need for it, why should every app using glib have to define this themselves?

> I am with Owen in believing that naked C
> expressions like
> 
> if (*s == '\0') 
> 
> or 
> 
> if (s == NULL || *s == '\0')
> 
> are nicer when dealing with C strings.

We're also not forcing people to use the new macro. It is a convenience.
 
> Not sure where this desire to wrap everything in a macro comes. 

Is there some reason you're saying this? This is the first time I have requested a macro (personally) in GLib, are others putting in similar requests?

I don't often use macros (in fact hardly ever), but when I do the same thing over and over again and see a way to make the code simpler to read and write, I generally optimise it out with something like this.

> It only makes
> you look up the macro definition every time to double-check whether it accepts
> NULL or not. 

I couldn't disagree more :)

How often do you look up how the ABS macro works? or any other macro that you use commonly? Also, as said before, people don't have to use it any more than they do any other convenience macro in GLib (CLAMP, MIN, MAX, etc)
 
> If you really need this macro, you can easily add it to your
> g-things-glib-maintainers-stubbornly-refused.h header.

Many applications already do this.

Perhaps this is a sweeping generalisation :) but I suspect the people that want this are involved more in UI development. At least, I haven't needed this sort of functionality so much since I was last writing UI based applications. There are some other exceptions too, where a lot of string handling is done. However, the point is still the same, in many cases, there is no difference between an empty string/null pointer when it comes to evaluating if a string should be used or not.

I really wonder what real objection there is here other than coding preference? 

Perhaps the issue is the macro name itself (as Owen suggested it could be named something else to make its function clearer)?
Comment 15 Christian Hergert 2010-09-10 15:41:02 UTC
I think having this as g_str_empty/g_str_empty0 would be easily recognizable by anyone using glib and fit the semantics we are used to with functions like g_strcmp0, etc.
Comment 16 Martyn Russell 2010-09-10 16:11:58 UTC
(In reply to comment #15)
> I think having this as g_str_empty/g_str_empty0 would be easily recognizable by
> anyone using glib and fit the semantics we are used to with functions like
> g_strcmp0, etc.

That's not a bad idea.

(In reply to comment #14)
> Perhaps the issue is the macro name itself (as Owen suggested it could be named
> something else to make its function clearer)?

After more discussion internally with Sven Herzberg and Tim Janik, it seems Tim tends to agree with my view here and Sven doesn't. However, Sven and I did come to the conclusion that the *name* is what is likely wrong here.

In either case, we could agree that the macro was useful if renamed to something more appropriate like G_STRP_HAS_DATA.
Comment 17 Tim Janik 2010-09-10 16:25:01 UTC
Just to throw in a few more comments here ;)

I think having a check in GLib for empty strings that is *short* enough to replace (s == NULL || s[0] == 0) would be a good thing to provide and recommend for these reasons:
- It promotes defensive programming so code meant to cope with 0-length strings won't blow up if it hits NULL instead, so this is reducing likelyness of crashes by some percentage.
- Treating NULL different from "" for strings is an idiom that works in C only, e.g. a C++ std::string can only represent "" but not a NULL pointer (because it's not pointing to a string). So pretending NULL and "" are (semantically) different in C is prone to producing API portability problems (regardless of the two being technically distinct).
- Use of NULL as a string should be generally discouraged for the above reasons, but not at the expense of instability, thus it makes sense to provide a macro that treats both the same way.

The key to a useful macro here is brevity though, otherwise nobody will use it. If we bent the namespace conventions in GLib for this particular case, we could be as short as:
  #define g_0str(s)      (s == NULL || s[0] == 0)
Comment 18 Xavier Claessens 2010-09-14 08:31:33 UTC
telepathy-glib defines that in its utils.h:

#define tp_str_empty(s) (tp_str_empty_inline (s))

static inline gboolean
tp_str_empty_inline (const gchar *s)
{
  return (s == NULL || s[0] == '\0');
}

s/tp/g/ and that's it :)

Note that in QT there are QString::isEmpty() and QString::isNull(). isEmpty() has the semantic to return true if the string is NULL or "".
Comment 19 Xavier Claessens 2012-04-27 10:29:54 UTC
Matthias Clasen: since your rejections are never final (bug #674634 comment #11), and 2nd though on this?

tp_str_empty0() could make it explicit that it's NULL-safe, like g_strcmp0 (btw, I would like a g_str_equal0 as well).
Comment 20 Xavier Claessens 2013-04-15 15:14:21 UTC
Created attachment 241572 [details] [review]
Add g_str_empty()

Copied from telepathy-glib's tp_str_empty().
Comment 21 Martyn Russell 2013-04-15 16:30:07 UTC
So, I wrote this patch and submitted it years ago when it was in Gossip.

I used a #define because there are a bunch of macros in Glib like this written with the preprocessor even though I prefer to be able to step through code in a debugger, so you're version is better in that respect Xavier :)

But please, is there some reason you reworked my patch with your name on it especially when it makes no semantic difference to the end user/developer whom uses it?

Regarding the NULL safe argument, you mean to change the name to make it clearer like other APIs right? I personally would always be wondering what the version without the 0 does as it's usually complimentary, but that's me.

I've also thought this patch might make more sense if it wasn't purely about strings, for example:

  g_mem_empty()

In many cases, you want to know if the pointer and what it points to has useful information. Perhaps this would be more acceptable by the GLib maintainers.

In either case, I would be surprised if this gets through because both Owen and Matthias have rejected this in the past.
Comment 22 Xavier Claessens 2013-04-15 18:00:02 UTC
Martyn: no particular reason, I just pushed lots of utility functions from tp-glib, I'm not the author of that tp_str_empty(). tbh I don't really care which patch goes in, but I think we really need some version of it.

About g_mem_empty(), I think its name makes it harder to find in devhelp, nobody will think about searching for that name. Is it common for non-string to be ended with '\0' ?

g_str_[is_]empty[0]() variants would be my preferred naming.
Comment 23 Allison Karlitskaya (desrt) 2013-04-16 12:33:14 UTC
I'm really not a fan of functions that increase people's tendency to conflate the empty string with NULL.  I'd therefore be against a g_str_is_empty() that doesn't crash on NULL.  g_str_is_empty0() is weird too because I'd expect the NULL case there to return FALSE (because it is _not_ the empty string).

All in all, open-coding these things is almost always nicer....
Comment 24 Xavier Claessens 2013-04-16 12:55:54 UTC
tbh, from my experience, there are not much case where NULL != "" really matters.

Typical use case: gtk_entry_get_text() does it return "" or NULL when user did not type anything? the doc does not even tell it !!! and it does not matter AT ALL, I would just use if (g_str_empty()) and that would be perfectly readable.
Comment 25 Xavier Claessens 2013-04-16 12:57:34 UTC
tbh, if your code makes a difference between NULL and "", it will crash, sooner or later.
Comment 26 Xavier Claessens 2013-04-16 13:04:30 UTC
oh, and the difference between "" and NULL is not bindable, and can't be sent over dbus. So actually, more I think about it, the less I'm capable of think about a situation where ""!=NULL matters. It seems just wrong practice actually.
Comment 27 Dan Winship 2013-04-16 13:23:36 UTC
Don't get carried away. There are certainly contexts where NULL and "" are equivalent, but there are also contexts where an empty string is just as valid as a non-empty string, and is entirely different from "no string at all". Eg, the URL "http://foo@example.com/" does not specify a password (password == NULL), but "http://foo:@example.com/" explicitly specifies a password of "".
Comment 28 Allison Karlitskaya (desrt) 2013-04-16 13:25:23 UTC
Your last three comments have done nothing but convince me even more that people confusing NULL and "" is a serious problem.

To me, at least, gtk_entry_get_text() _obviously_ returns the empty string because there's zero characters there.  If I go from 2 to 1 character when pressing backspace then obviously I go from 1 to 0 when pressing it again... and a zero-length string is not the same as NULL.

And NULL vs. '' binds just fine.  In python that's None vs. ''.  In Vala and C#, that's 'string?' where there is a difference between the two is as plain as it is in C.  It's true that we lack maybe types in D-Bus, but that's a deficiency of D-Bus.

I take good care to keep the distinction between NULL and "" clear, and my code doesn't 'crash sooner or later' because of it.  Just the opposite, I think.
Comment 29 Xavier Claessens 2013-04-16 13:47:47 UTC
(In reply to comment #27)
> Don't get carried away. There are certainly contexts where NULL and "" are
> equivalent, but there are also contexts where an empty string is just as valid
> as a non-empty string, and is entirely different from "no string at all". Eg,
> the URL "http://foo@example.com/" does not specify a password (password ==
> NULL), but "http://foo:@example.com/" explicitly specifies a password of "".

Well, if you consider empty password a valid password... But indeed in this case it could matter.

(In reply to comment #28)
> And NULL vs. '' binds just fine.  In python that's None vs. ''.  In Vala and
> C#, that's 'string?' where there is a difference between the two is as plain as
> it is in C.  It's true that we lack maybe types in D-Bus, but that's a
> deficiency of D-Bus.

Actually didn't know python does that difference. but in python
if not str: will be fine with both None and "".
Comment 30 Murray Cumming 2013-04-17 07:00:00 UTC
With no expectation whatsoever of changing the decision here, here is my perspective as the gtkmm maintainer:

In C++, using the std::string type, we have no opportunity to distinguish between no string and empty string in one parameter. For that we generally have explicitly separate methods (sometimes with method overloads, using different numbers of parameters, effectively like vala's ?) or explicitly separate parameters to indictate whatever special case is meant by NULL. I consider these to be better APIs.

In glib and GTK+ we've often encountered real problems when the code hasn't had any thought given to what happens if "" rather than NULL (or vice-versa) is supplied as a parameter. We've had things like tooltips showing up that are empty, rather than no tooltip, which isn't of any use to anyone. We generally have a hard time getting these fixed inside GTK+ because the wish to distinguish NULL and "" seems more important than the wish to avoid useless (or counter-intuitive) end results.

Not distinguishing NULL and "" seems to be the more common case. Adding a helper function would help that common case, making that common case be more consistently handled, and making it more obvious (by not using it) where NULL should have a special meaning, though you'd still need documentation to describe that special meaning.

For me, it feels a bit like the old resistance to having TRUE and FALSE macros.

People will add this macro to their application and library code, but it would have been nice to see this actually used inside glib and GTK+.
Comment 31 Xavier Claessens 2013-04-17 08:07:12 UTC
If NULL was not a string, gtk_label_new(NULL) should at least print warning. But instead it does the str ? str : "" trick. Which is nice convenient trick because we don't care about the difference in almost every case.
Comment 32 Matthias Clasen 2013-04-17 10:18:48 UTC
Seems clear that we (as in Ryan, Owen and me) still don't want this in glib.
Comment 33 Martyn Russell 2013-04-17 20:14:27 UTC
(In reply to comment #23)
> I'm really not a fan of functions that increase people's tendency to conflate
> the empty string with NULL.  I'd therefore be against a g_str_is_empty() that
> doesn't crash on NULL.

This assumes that everyone thinks about and uses strings the same way you do OR that they would misunderstand the difference between NULL and "". That's simply not true.

I also think a library like GLib should be protective with its APIs and don't really understand *why* you would expect a function like this or any other to crash on NULL?

>  g_str_is_empty0() is weird too because I'd expect the
> NULL case there to return FALSE (because it is _not_ the empty string).

I agree.

> All in all, open-coding these things is almost always nicer....

This really reinforces my first point, which is that when you write certain types of programs (e.g. GTK+ applications) and actually only care about 1 thing: "is there data or not", this function becomes very useful.

I've found this sort of function less useful, in my experience, when writing libraries and daemons. I am generalising there, but I don't think GLib maintainers should be generalising about how people perceive or use NULL & "" especially with regard to this API if it is documented properly.

For me, this is about avoiding 2 checks EVERY time you use a widget or function that could be returning 2 different things which ultimately mean 1 same thing for the developer in a common circumstance.

(In reply to comment #30)
> Not distinguishing NULL and "" seems to be the more common case. Adding a
> helper function would help that common case, making that common case be more
> consistently handled, and making it more obvious (by not using it) where NULL
> should have a special meaning, though you'd still need documentation to
> describe that special meaning.

[snip]

> People will add this macro to their application and library code, but it would
> have been nice to see this actually used inside glib and GTK+.

+1.

I've seen this macro replicated a bunch of times in GNOME's sources over the years and think it's crazy that it's being objected to when it's such a simple/small addition. As far as I can tell, the downside of this addition is that people might use it to check for NULL or "" exclusively when it can mean either. That's simply an API interpretation error, not a NULL vs "" misunderstanding.

Xavier, I thought you made some good points too.

Dan, I don't expect people who differentiate between "" and NULL (e.g. your password case) to be using this function anyway and if they did, they should know what the function does before using it. That shouldn't preclude people who need a function like this from having it available to them IMO.
Comment 34 Xavier Claessens 2013-10-23 18:13:19 UTC
As extra motivation for this, I was reading gnome-contacts code, in vala, and was wondering what's "is_set" everywhere... then saw:

  private static bool is_set (string? str) {
    return str != null && str != "";
  }

Written by Alexander Larsson.

I think literally every app reimplement this. Would be easier if glib provided a function/macro for that instead of seeing different names in each app.
Comment 35 Martyn Russell 2013-10-24 13:30:41 UTC
I noticed the same thing in libmediaart (new library I am working on based on Tracker sources). I am thinking about defining a macro there again :/

It's clearly useful.