Hi,
Jeff King wrote:
> On Sun, Oct 14, 2018 at 04:29:06PM +0200, René Scharfe wrote:
>> Anyway, drove the generative approach a bit further, and came up with
>> the new DEFINE_SORT below. I'm unsure about the name; perhaps it should
>> be called DEFINE_SORT_BY_COMPARE_FUNCTION_BODY, but that's a bit long.
>> It handles casts and const attributes behind the scenes and avoids
>> repetition, but looks a bit weird, as it is placed where a function
>> signature would go.
>>
>> Apart from that the macro is simple and doesn't use any tricks or
>> added checks. It just sets up boilerplate functions to offer type-safe
>> sorting.
>>
>> diffcore-rename.c and refs/packed-backend.c receive special treatment in
>> the patch because their compare functions are used outside of sorting as
>> well. I made them take typed pointers nevertheless and used them from
>> DEFINE_SORT; the wrapper generated by that macro is supposed to be
>> private. Given that such reuse is rare and I think we don't need a way
>> to make it public.
>>
>> What do y'all think about this direction?
>
> I think it's the best we're likely to do, and is an improvement on the
> status quo.
>
> The patch looks overall sane to me. I think DEFINE_SORT() is a fine
> name.
Since this came up in [1], I took a glance at this.
I also think it looks reasonable, though it's possible to do better if
we're willing to (1) cast between pointers to function with different
signatures, which is portable in practice but I don't believe the C
standard speaks to and (2) conditionally make use of gcc extensions,
for typechecking.
For example, CCAN's asort[2] does typechecking on the arrays passed in
and the callback cookie parameter to qsort_r, with no extra
boilerplate or run-time overhead involved[3].
(The core of that macro is ccan's typesafe_cb_cast[4]:
/* CC0 (Public domain) - see LICENSE file for details */
#if HAVE_TYPEOF && HAVE_BUILTIN_CHOOSE_EXPR && HAVE_BUILTIN_TYPES_COMPATIBLE_P
/**
* typesafe_cb_cast - only cast an expression if it matches a given type
* @desttype: the type to cast to
* @oktype: the type we allow
* @expr: the expression to cast
*
* This macro is used to create functions which allow multiple types.
* The result of this macro is used somewhere that a @desttype type is
* expected: if @expr is exactly of type @oktype, then it will be
* cast to @desttype type, otherwise left alone.
*
* This macro can be used in static initializers.
*
* This is merely useful for warnings: if the compiler does not
* support the primitives required for typesafe_cb_cast(), it becomes an
* unconditional cast, and the @oktype argument is not used. In
* particular, this means that @oktype can be a type which uses the
* "typeof": it will not be evaluated if typeof is not supported.
*
* Example:
* // We can take either an unsigned long or a void *.
* void _set_some_value(void *val);
* #define set_some_value(e) \
* _set_some_value(typesafe_cb_cast(void *, unsigned long, (e)))
*/
#define typesafe_cb_cast(desttype, oktype, expr) \
__builtin_choose_expr( \
__builtin_types_compatible_p(__typeof__(0?(expr):(expr)), \
oktype), \
(desttype)(expr), (expr))
#else
#define typesafe_cb_cast(desttype, oktype, expr) ((desttype)(expr))
#endif
)
Thanks,
Jonathan
[1] https://lore.kernel.org/git/20201117223011.GA642234@xxxxxxxxxxxxxxxxxxxxxxx/
[2] https://git.ozlabs.org/?p=ccan;a=blob;f=ccan/asort/asort.h;hb=HEAD
[3] https://ccodearchive.net/info/asort.html
[4] https://git.ozlabs.org/?p=ccan;a=blob;f=ccan/typesafe_cb/typesafe_cb.h;hb=HEAD
