Created attachment 303361 [details] [review]
Use a GHashTable as a set when possible

Review of attachment 303361 [details] [review]:

You should probably use g_hash_table_add() instead of insert() with key == value.

Created attachment 303376 [details] [review]
Use a GHashTable as a set when possible

(In reply to Emmanuele Bassi (:ebassi) from comment #2)
> Review of attachment 303361 [details] [review] [review]:
> 
> You should probably use g_hash_table_add() instead of insert() with key ==
> value.

Fixed.

Created attachment 303377 [details] [review]
Use g_hash_table_add() where possible

Review of attachment 303376 [details] [review]:

You should also convert g_hash_table_lookup() calls into g_hash_table_contains() calls, to make it really obvious the hash table is being used as a set.

::: tests/testglib.c
@@ -1054,3 @@
     {
       array[i] = i;
-      g_hash_table_insert (hash_table, &array[i], &array[i]);

Since this is a test suite, does it make sense to convert the calls here? I’d leave this alone, and rely on the tests in glib/tests/hash.c to cover the set-like functions.

Review of attachment 303377 [details] [review]:

Why not merge this into the first patch?

Similarly, I think the changes here should also include use of g_hash_table_contains().

::: gio/gresource.c
@@ +809,2 @@
           for (i = 0; children[i] != NULL; i++)
+            g_hash_table_add (hash, children[i]);

(Looks like this one’s already been changed in master.)

::: glib/deprecated/grel.c
@@ +307,3 @@
   va_end (args);
   
+  g_hash_table_add (relation->all_tuples, tuple);

The way that GRelation uses hash tables is a little more complex than the other calling sites (use of keys and values in g_hash_table_foreach() callback functions). Since it’s deprecated, it might make sense to omit the changes in grel.c.

Ping?

Created attachment 362242 [details] [review]
Use hash tables as sets in various places

Where we were already treating GHashTables as sets, modify them to use
the set-specific APIs g_hash_table_add() and g_hash_table_contains(), to
make that usage more obvious and less prone to being broken.

Heavily based on patches by Garrett Regier <garrettregier@gmail.com>.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

I squashed the two patches together, dropped the changes to testglib and GRelation, and fixed my other review comments. Ready for review.

Review of attachment 362242 [details] [review]:

looks fine to me

Attachment 362242 [details] pushed as 3eacec1 - Use hash tables as sets in various places