GNOME Bugzilla – Bug 697828
g_hash_table_add() should return a boolean
Last modified: 2013-11-24 15:38:57 UTC
g_hash_table_add() should return TRUE if the element was not yet in the table, and FALSE otherwise. _insert() and _replace() could do the same. That would allow doing things like: if (g_hash_table_add (table, foo)) g_signal_emit (self, signals[ADDED], 0, foo);
Currently I have to do 2 lookups: if (g_hash_table_contains (table, foo)) return; g_hash_table_add (table, foo); g_signal_emit (self, signals[ADDED], 0, foo);
I'm not opposed to this idea. But it is a little more tricky for replace - what should true mean here ? that is was replaced ? in that case, it kinda means the opposite than it does for insert, where true means 'was not in the table yet'.
Note that I can already do if (g_hash_table_remove (table, foo)) g_signal_emit(); so doing the same on _add() would make it more consistent. About _replace() VS _insert() that's actually something I've never understood. In which case would I want to use _insert() instead of _replace()? Indeed the meaning of the returned boolean needs to be carefully defined and could be counter-intuitive... I would say TRUE means "a value was already there" so _insert() and _replace() would always return the same.
Created attachment 241571 [details] [review] GHashTable: return boolean in _add(), _insert() and _replace()
I prefer to handle this by improving the GHashTableIter API, which would allow quite a lot of really interesting uses and would also avoid the double-lookup you are annoyed by here. mclasen muses about some possibilities in bug 630336.
*** Bug 668659 has been marked as a duplicate of this bug. ***
In my patch the doc is more complete, I think it is important to at least tell since which version there is a return value.