Re: [PATCH 15/16] commit-reach: make can_all_from_reach... linear
- From
René Scharfe <l.s.r@web.de>
- Date
- Oct 15, 2018, 16:26 UTC
- Message-ID
- <cd1b0e5f-94b0-4905-aee8-edec00dc0a49@web.de>
- In-Reply-To
- <0cd89b69-4b65-f961-a025-c74851f8c999@gmail.com>
Am 15.10.2018 um 17:31 schrieb Derrick Stolee:
Show 30 quoted lines
> On 10/14/2018 10:29 AM, René Scharfe wrote:
>> diff --git a/git-compat-util.h b/git-compat-util.h
>> index 5f2e90932f..491230fc57 100644
>> --- a/git-compat-util.h
>> +++ b/git-compat-util.h
>> @@ -1066,6 +1066,21 @@ static inline void sane_qsort(void *base, size_t nmemb, size_t size,
>> qsort(base, nmemb, size, compar);
>> }
>>
>> +#define DECLARE_SORT(scope, name, elemtype) \
>> +scope void name(elemtype, size_t)
>> +
>> +#define DEFINE_SORT(scope, name, elemtype, one, two) \
>> +static int name##_compare(const elemtype, const elemtype); \
>> +static int name##_compare_void(const void *a, const void *b) \
>> +{ \
>> + return name##_compare(a, b); \
>> +} \
>> +scope void name(elemtype base, size_t nmemb) \
>> +{ \
>> + QSORT(base, nmemb, name##_compare_void); \
>> +} \
>> +static int name##_compare(const elemtype one, const elemtype two)
>> +
>
> Since you were worried about the "private" name of the compare function,
> maybe split this macro into two: DEFINE_COMPARE and DEFINE_SORT. Then,
> if someone wants direct access to the compare function, they could use
> the DEFINE_COMPARE to ensure the typing is correct, and use QSORT as
> normal with name##_compare_void.The pointers are converted to const void * somewhere along the way from qsort() to compare function. Splitting the macro would require type check tricks to make sure the types of the compare function matches the array to be sorted. Letting a single macro bake it all into a layer cake of generated functions is a lot simpler.
Show 6 quoted lines
> As I think about this, I think this is less of a problem than is worth > this split. The commit-slab definitions generate a lot of methods using > the "name##" convention, so perhaps we should just trust developers > using the macros to look up the macro definition or similar examples. In > that sense, including a conversion that consumes the compare function > directly can be a signpost for future callers.
Using the generated compare function name directly is a bit awkward; e.g. in the two example cases it would be sort_by_score_compare() and sort_packed_ref_records_compare(). Defining the real compare function the usual way (with a proper name) and having the DEFINE_SORT block call it is a bit more repetitive, but clean and understandable IMHO.
We also could just leave complicated cases alone..
> I would say that maybe the times where you need to do something special > should be pulled out into their own patches, so we can call attention to > them directly.
Right; this patch was just a sketch.
Show 25 quoted lines
> I like this "sort_by_" convention..
>
>>
>> for (i = 0; i < nr_packs; i++) {
>> pack_names[i] = pairs[i].pack_name;
>> @@ -455,10 +453,8 @@ struct pack_midx_entry {
>> uint64_t offset;
>> };
>>
>> -static int midx_oid_compare(const void *_a, const void *_b)
>> +DEFINE_SORT(static, sort_midx, struct pack_midx_entry *, a, b)
>> {
>> - const struct pack_midx_entry *a = (const struct pack_midx_entry *)_a;
>> - const struct pack_midx_entry *b = (const struct pack_midx_entry *)_b;
>> int cmp = oidcmp(&a->oid, &b->oid);
>>
>> if (cmp)
>> @@ -573,7 +569,7 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,
>> }
>> }
>>
>> - QSORT(entries_by_fanout, nr_fanout, midx_oid_compare);
>> + sort_midx(entries_by_fanout, nr_fanout);
>
> ...but it isn't followed here. Perhaps "sort_by_oid"?That function sorts by oid, pack_mtime, and pack_int_id, but including all these fields in the name is a bit unwieldy. Being unspecific by calling it sort_midx() was the lazy way out. Mentioning only oid is a bit misleading. Perhaps sort_by_oid_etc()?
René