Re: [PATCH 1/3] add QSORT
- From
René Scharfe <l.s.r@web.de>
- Date
- Sep 29, 2016, 23:40 UTC
- Message-ID
- <302c140e-bfb9-a54c-7ce0-14b266611584@web.de>
- In-Reply-To
- <eeb2791e-c09b-b7bc-f8e8-336d4f3906b8@web.de>
Am 30.09.2016 um 01:21 schrieb René Scharfe:
Show 74 quoted lines
> Am 30.09.2016 um 00:36 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> Add the macro QSORT, a convenient wrapper for qsort(3) that infers the
>>> size of the array elements and supports the convention of initializing
>>> empty arrays with a NULL pointer, which we use in some places.
>>>
>>> Calling qsort(3) directly with a NULL pointer is undefined -- even with
>>> an element count of zero -- and allows the compiler to optimize away any
>>> following NULL checks. Using the macro avoids such surprises.
>>>
>>> Add a semantic patch as well to demonstrate the macro's usage and to
>>> automate the transformation of trivial cases.
>>>
>>> Signed-off-by: Rene Scharfe <l.s.r@web.de>
>>> ---
>>> contrib/coccinelle/qsort.cocci | 19 +++++++++++++++++++
>>> git-compat-util.h | 8 ++++++++
>>> 2 files changed, 27 insertions(+)
>>> create mode 100644 contrib/coccinelle/qsort.cocci
>>
>> The direct calls to qsort(3) that this series leaves behind are
>> interesting.
>>
>> 1. builtin/index-pack.c has this:
>>
>> if (1 < opts->anomaly_nr)
>> qsort(opts->anomaly, opts->anomaly_nr, sizeof(uint32_t), cmp_uint32);
>>
>> where opts->anomaly is coming from pack.h:
>>
>> struct pack_idx_option {
>> unsigned flags;
>> ...
>> int anomaly_alloc, anomaly_nr;
>> uint32_t *anomaly;
>> };
>>
>> I cannot quite see how the automated conversion misses it? It's not
>> like base and nmemb are type-restricted in the rule (they are both
>> just "expression"s).
>>
>> 2. builtin/shortlog.c has this:
>>
>> qsort(log->list.items, log->list.nr, sizeof(struct string_list_item),
>> log->summary ? compare_by_counter : compare_by_list);
>>
>> where log->list is coming from shortlog.h:
>>
>> struct shortlog {
>> struct string_list list;
>> };
>>
>> and string-list.h says:
>>
>> struct string_list {
>> struct string_list_item *items;
>> unsigned int nr, alloc;
>> ...
>> };
>>
>> which seems to be a good candidate for this rule:
>>
>> type T;
>> T *base;
>> expression nmemb, compar;
>> @@
>> - qsort(base, nmemb, sizeof(T), compar);
>> + QSORT(base, nmemb, compar);
>>
>> if we take "T == struct string_list_item".
>
> Transformations for these two are generated if we pass --all-includes
> to spatch. So let's do that.And here's the result:
-- >8 -- Subject: [PATCH] use QSORT, part 2
Convert two more qsort(3) calls to QSORT to reduce code size and for better safety and consistency.
Signed-off-by: Rene Scharfe <l.s.r@web.de> --- Squashable.
builtin/index-pack.c | 3 +-- builtin/shortlog.c | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 7657d0a..0a27bab 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1531,8 +1531,7 @@ static void read_v2_anomalous_offsets(struct packed_git *p, opts->anomaly[opts->anomaly_nr++] = ntohl(idx2[off * 2 + 1]); } - if (1 < opts->anomaly_nr) - qsort(opts->anomaly, opts->anomaly_nr, sizeof(uint32_t), cmp_uint32); + QSORT(opts->anomaly, opts->anomaly_nr, cmp_uint32); } static void read_idx_option(struct pack_idx_option *opts, const char *pack_name) diff --git a/builtin/shortlog.c b/builtin/shortlog.c index 25fa8a6..ba0e115 100644 --- a/builtin/shortlog.c +++ b/builtin/shortlog.c @@ -308,7 +308,7 @@ void shortlog_output(struct shortlog *log) struct strbuf sb = STRBUF_INIT; if (log->sort_by_number) - qsort(log->list.items, log->list.nr, sizeof(struct string_list_item), + QSORT(log->list.items, log->list.nr, log->summary ? compare_by_counter : compare_by_list); for (i = 0; i < log->list.nr; i++) { const struct string_list_item *item = &log->list.items[i];
-- 2.10.0