Re: [PATCH 15/16] commit-reach: make can_all_from_reach... linear

From: Jeff King <peff@peff.net>
Date: Oct 17, 2018, 08:33 UTC
Message-ID: <20181017083340.GB31932@sigill.intra.peff.net>
In-Reply-To: <c141fb44-904f-e8b6-119f-7d2d6bcfd81a@web.de>

On Sun, Oct 14, 2018 at 04:29:06PM +0200, René Scharfe wrote:

Show 19 quoted lines

> 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.

I think given a macro parameter "foo" you could generate sort_by_foo() and compare_foo(), which would eliminate the extra layer in those two cases you mentioned. But I'm also fine with the approach you've shown here.

-Peff