Am 01.10.2018 um 21:26 schrieb Derrick Stolee:
> On 10/1/2018 3:16 PM, René Scharfe wrote:
>> Am 28.06.2018 um 14:31 schrieb Derrick Stolee via GitGitGadget:
>>> diff --git a/commit-reach.c b/commit-reach.c
>>> index c58e50fbb..ac132c8e4 100644
>>> --- a/commit-reach.c
>>> +++ b/commit-reach.c
>>> @@ -513,65 +513,88 @@ int commit_contains(struct ref_filter *filter, struct commit *commit,
>>> return is_descendant_of(commit, list);
>>> }
>>>
>>> -int reachable(struct commit *from, int with_flag, int assign_flag,
>>> - time_t min_commit_date)
>>> +static int compare_commits_by_gen(const void *_a, const void *_b)
>>> {
>>> - struct prio_queue work = { compare_commits_by_commit_date };
>>> + const struct commit *a = (const struct commit *)_a;
>>> + const struct commit *b = (const struct commit *)_b;
>> This cast is bogus. QSORT gets handed a struct commit **, i.e. an array
>> of pointers, and qsort(1) passes references to those pointers to the
>> compare function, and not the pointer values.
>
> Good catch! I'm disappointed that we couldn't use type-checking here, as
> it is quite difficult to discover that the types are wrong here.
Generics in C are hard, and type checking traditionally falls by the
wayside. You could use macros for that, like klib [*] does, but that
has its own downsides (more object text, debugging the sort macros
themselves is harder).
[*] https://github.com/attractivechaos/klib/blob/master/ksort.h
>> diff --git a/commit-reach.c b/commit-reach.c
>> index 00e5ceee6f..2f5e592d16 100644
>> --- a/commit-reach.c
>> +++ b/commit-reach.c
>> @@ -529,8 +529,8 @@ int commit_contains(struct ref_filter *filter, struct commit *commit,
>>
>> static int compare_commits_by_gen(const void *_a, const void *_b)
>> {
>> - const struct commit *a = (const struct commit *)_a;
>> - const struct commit *b = (const struct commit *)_b;
>> + const struct commit *a = *(const struct commit * const *)_a;
>> + const struct commit *b = *(const struct commit * const *)_b;
>
> I would expect s/* const */**/ here, but I'm guessing your formulation
> is a bit extra careful about types.
Yeah, that second const is not necessary, as the dereference in the same
line makes it inconsequential, but I added it to make clear that this
function is really not supposed to write at all..
René
