On 01/25/16 10:27, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> writes: > >> On 01/22/2016 12:24 PM, Markus Armbruster wrote: >>> Eric Blake <ebl...@redhat.com> writes: >>> >>>> When reporting that an unvisited member remains at the end of an >>>> input visit for a struct, we were using g_hash_table_find() >>>> coupled with a callback function that always returns true, to >>>> locate an arbitrary member of the hash table. But if all we >>>> need is an arbitrary entry, we can get that from a single-use >>>> iterator, without needing a tautological callback function. >>> >>> Good idea. >>> >>>> Suggested-by: Markus Armbruster <arm...@redhat.com> >>> >>> Whoops, it's even mine! I forgot... %-)
Side remark: https://developer.gnome.org/glib/stable/glib-Hash-Tables.html g_hash_table_find (): Since: 2.4 g_hash_table_iter_next (): Since: 2.16 I can't prove that when I wrote this code, g_hash_table_iter_next() wasn't available, but I may easily have googled & looked at GLib *documentation* that lacked g_hash_table_iter_next(). :) >>> >>>> Signed-off-by: Eric Blake <ebl...@redhat.com> >>>> Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com> >>>> >>>> --- >> >>>> GQueue *any; >>>> >>>> if (--ov->depth > 0) { >>>> @@ -174,8 +168,8 @@ opts_end_struct(Visitor *v, Error **errp) >>>> } >>>> >>>> /* we should have processed all (distinct) QemuOpt instances */ >>>> - any = g_hash_table_find(ov->unprocessed_opts, &ghr_true, NULL); >>>> - if (any) { >>>> + g_hash_table_iter_init(&iter, ov->unprocessed_opts); >>>> + if (g_hash_table_iter_next(&iter, NULL, (void **)&any)) { >>> >>> Is this cast kosher? >> >> You may have a point that it violates some corner of C99, but I'm not >> the first such user: >> >> hw/i386/intel_iommu.c: while (g_hash_table_iter_next (&bus_it, NULL, >> (void**)&vtd_bus)) { >> qom/object.c: while (g_hash_table_iter_next(&iter, NULL, (gpointer >> *)&prop)) { >> >> Conceptually, it seems fine - void* can be assigned to any pointer, and >> 'any' qualifies as such a pointer. We are then taking the address of >> that (or void**) to pass by reference. > > Yes, any pointer can be converted to and from void *. Doesn't mean the > conversion results in the same bits. It does on machines with a single, > uniform pointer format, i.e. any sane machine. > > (void **)iter converts iter, not *iter. *iter gets reinterpreted on > dereference. > > You're right in that this kind of type punning already occurs in QEMU. > Also in other programs. We can hope it's common enough to deter > optimizers from screwing it up even on sane machines. > >> I suppose a stricter version would be: >> >> void *wrap; >> GQueue *any; >> if (g_hash_table_iter_next(&iter, NULL, &wrap)) { >> any = wrap; >> ... >> >> but is it worth the bother? Put another way, will a compiler ever do >> the wrong thing to us because C99 might have some corner case? Laszlo, >> what's your take? > > Perhaps Laszlo can confirm or refute my reading of the standard. The concern is justified; the expression "(void **)&any" does not convert a pointer-to-void to pointer-to-GQueue, it causes the bit pattern to be reinterpreted (it is stored as pointer-to-void and reinterpreted as pointer-to-GQueue). 6.2.5 Types p27 says "A pointer to void shall have the same representation and alignment requirements as a pointer to a character type. [...] All pointers to structure types shall have the same representation and alignment requirements as each other." (I'll assume that GQueue is a struct.) But, they need not match each other. At worst, "any" might not even have enough storage for a pointer-to-void. I'll mention that edk2 is chock-full of the above style cast. I think even the UEFI spec is, in code examples. But, edk2 builds with -fno-strict-aliasing & friends. ... I think it doesn't matter in practice, but if we're trying to prevent compilers from outsmarting us, I'd feel better about "any = wrap". Thanks Laszlo > >>>> if (top_ht) { >>>> - if (g_hash_table_size(top_ht)) { >>>> - const char *key; >>>> - g_hash_table_find(top_ht, always_true, &key); >>>> + GHashTableIter iter; >>>> + const char *key; >>>> + >>>> + g_hash_table_iter_init(&iter, top_ht); >>>> + if (g_hash_table_iter_next(&iter, (void **)&key, NULL)) { >>> >>> Is this cast kosher? >> >> Here, in addition to the above argument, we also needed to cast away const.
