Re: [Libguestfs] [libnbd PATCH v3 05/29] vector: (mostly) factor out DEFINE_VECTOR_EMPTY

On 2/15/23 21:27, Eric Blake wrote: > On Wed, Feb 15, 2023 at 03:11:34PM +0100, Laszlo Ersek wrote: >> The "name##_iter" function is used 11 times in libnbd; in all those cases, >> "name" is "string_vector", and the "f" callback is "free": >> >> string_vector_iter (..., (void *) free); >> >> Casting "free" to (void*) is ugly. (Well-defined by POSIX, but still.) > > Tangentially related: casting function pointers in general may get > harder as more compilers move towards C23 and its newer rules (see for > example > https://lists.gnu.org/archive/html/bug-gnulib/2023-02/msg00055.html or > this gcc 13 bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108694 > which highlights some of the warnings that newer compilers will start > warning us about). While this patch focuses on avoiding casts between > fn(*)(type*) and void*, I don't know off-hand if C23 will permit > fn(*)(void*) as a compatible function pointer with fn(*)(type). My understanding is that, per C99 at least, ret_type(*)(void*) is compatible with ret_type(*)(type*) precisely if void* is compatible with type* (6.7.5.3p15). Whether void* is compatible with type* depends on... ugh, that's hard to deduce from the standard. 6.7.5.1p2 says, "For two pointer types to be compatible, both shall be identically qualified and both shall be pointers to compatible types". I don't think "void" (as a type in itself) is compatible with any type! Now, there is one particular statement on void* -- 6.2.5p27 says, "A pointer to void shall have the same representation and alignment requirements as a pointer to a character type." (I think the statements about *converting* void* to type* and vice versa do not apply here; AFAICT "compatibility" is about reinterpreting the bit patterns, not converting values.) In Annex I (Common warnings, "informative"), the following is listed: "An implicit narrowing conversion is encountered, such as the assignment of [...] a pointer to void to a pointer to any type other than a character type". All in all I don't think ret_type(*)(type*) is compatible with ret_type(*)(void*) in the general case, and that's why in this patch I didn't want to go more general than I absolutely needed to.

> > Thinking higher-level now, your new macro is something where we have > to do a two-step declaration of macro types where we want the new > function. Would it make more sense to change the signature of the > DEFINE_VECTOR_TYPE() macro to take a third argument containing the > function name to call on cleanup paths, with the ability to easily > write/reuse a no-op function for vectors that don't need to call > free(), where we can then unconditionally declare name##_empty() that > will work with all vector types? That is, should we consider instead > doing something like: > > DEFINE_VECTOR_TYPE (string_vector, char *, free); > > DEFINE_VECTOR_TYPE (int_vector, int, noop); My counter-arguments: - this requires updates to all existent DEFINE_VECTOR_TYPE macro invocations, - with "noop" passed to _reset, _reset and _empty become effectively the same, so we get (at least partially) duplicate APIs, - this would be a step towards combinatorial explosion - if "noop" does nothing, then why call it on each element of the vector anyway? It's not only the function call that becomes superfluous in the loop bodym with the function being "noop", but the loop *itself* becomes superfluous. So then we might want to compare the function pointer against "noop" outside of the loop... and that way we get a bunch of complications :) I chose this approach because it is purely additive and precisely as generic/specific as it needs to be. We already have 11 use cases, so I don't think it's *too* specific.

From your other mail in this subthread:

>> +#define DEFINE_VECTOR_EMPTY(name) \ > > I'm going to be that guy ... Yes, someone has to be! I knew it was virtually impossible for me to get the name right at the first try :) > > Can we call it ADD_VECTOR_EMPTY_METHOD or similar/better? Eric, any comments?