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 592715 - Document that g_str_hash() and g_int_hash() are not NULL safe
Document that g_str_hash() and g_int_hash() are not NULL safe
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2009-08-22 15:49 UTC by Paul Bolle
Modified: 2011-10-04 13:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Trivial doc patch (1.12 KB, patch)
2009-08-22 15:51 UTC, Paul Bolle
needs-work Details | Review
[1/4] GHashTable: be more clear what g_int_hash wants (2.45 KB, patch)
2011-10-03 16:28 UTC, Simon McVittie
accepted-commit_now Details | Review
=2/4] Be completely clear about what g_direct_hash, g_direct_equal do (2.00 KB, patch)
2011-10-03 16:29 UTC, Simon McVittie
accepted-commit_now Details | Review
[3/4] Be clear that g_str_hash etc. don't accept NULL (4.47 KB, patch)
2011-10-03 16:29 UTC, Simon McVittie
accepted-commit_now Details | Review
[optional 4/4] Be really, pedantically, clear about NULL hash keys (3.17 KB, patch)
2011-10-03 16:30 UTC, Simon McVittie
accepted-commit_now Details | Review

Description Paul Bolle 2009-08-22 15:49:18 UTC
0) I'm spending some time to understand bug #592618. Progress is, as usual, slow.

1) A first (trivial) step in solving this bug would be to document that g_str_hash() and g_int_hash() are not NULL safe. Draft patch should follow shortly.
Comment 1 Paul Bolle 2009-08-22 15:51:27 UTC
Created attachment 141426 [details] [review]
Trivial doc patch
Comment 2 Paul Bolle 2009-08-23 10:13:18 UTC
In the meantime I've noticed some other *_hash functions (g_int64_hash() and g_double_hash()) and some *_equal functions (g_int_equal(), g_int64_equal(), g_double_equal()) that could benefit from a line (similar to the line used in the draft patch) about their NULL safety.

g_str_equal() has a line implicitly warning that it is not NULL safe. Maybe an explicit warning could be added there too.
Comment 3 Matthias Clasen 2011-05-02 15:33:44 UTC
Review of attachment 141426 [details] [review]:

Can you write a patch that covers all the affected equal and hash functions ?
Comment 4 Paul Bolle 2011-05-21 19:37:59 UTC
(In reply to comment #3)
> Can you write a patch that covers all the affected equal and hash functions ?

This issue was opened when I was still active on evolution. I don't think I have contributed to evolution since the beginning of 2010 (ie, in the last 18 months) and have actually contributed very little to the rest of gnome in that same period.

I'm afraid it's unlikely that I'll restart working on this, at least in the near future.
Comment 5 Simon McVittie 2011-10-03 16:28:54 UTC
Created attachment 198118 [details] [review]
[1/4] GHashTable: be more clear what g_int_hash wants

Using g_int_hash, g_int_equal with keys like GINT_TO_POINTER (n) seems to
be a reasonably common GLib-novice mistake. It doesn't help that the
documentation for GHashFunc was ambiguous about this.
Comment 6 Simon McVittie 2011-10-03 16:29:15 UTC
Created attachment 198119 [details] [review]
=2/4] Be completely clear about what g_direct_hash, g_direct_equal do

Also annotate them as (allow-none), more for the benefit of gtk-doc
readers than introspection.
Comment 7 Simon McVittie 2011-10-03 16:29:34 UTC
Created attachment 198120 [details] [review]
[3/4] Be clear that g_str_hash etc. don't accept NULL

This covers the str, double, int, int64 hash and equal functions, but not
anything that takes an "object", since the convention is that "object
methods" never accept NULL anyway.
Comment 8 Simon McVittie 2011-10-03 16:30:01 UTC
Created attachment 198121 [details] [review]
[optional 4/4] Be really, pedantically, clear about NULL hash keys

I don't think we need to go quite this far, but maybe we should...
Comment 9 Colin Walters 2011-10-03 18:28:18 UTC
Review of attachment 198118 [details] [review]:

Looks good, thanks!
Comment 10 Colin Walters 2011-10-03 19:32:36 UTC
Review of attachment 198119 [details] [review]:

Looks good.
Comment 11 Colin Walters 2011-10-03 19:33:31 UTC
Review of attachment 198120 [details] [review]:

Looks good.
Comment 12 Colin Walters 2011-10-03 19:34:31 UTC
Review of attachment 198121 [details] [review]:

People rarely complain about the length or correctness of documentation =)  If someone you know has hit this, fine by me to add.
Comment 13 Simon McVittie 2011-10-04 09:42:46 UTC
(In reply to comment #12)
> People rarely complain about the length or correctness of documentation =)  If
> someone you know has hit this, fine by me to add.

I think just saying "non-NULL" is enough, so I didn't push these.

I've pushed the other three to master for 2.31.0. Could you close the bug, or give me suitable permission bits to do so myself, please?