Re: [GSoC][PATCH v2 1/6] reftable: clean up reftable/pq.c
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Jun 10, 2024, 07:36 UTC
- Message-ID
- <ZmatCKg6gbN3Aran@tanuki>
- In-Reply-To
- <20240606154712.15935-2-chandrapratap3519@gmail.com>
On Thu, Jun 06, 2024 at 08:53:37PM +0530, Chandra Pratap wrote:
Show 47 quoted lines
> According to Documentation/CodingGuidelines, control-flow statements
> with a single line as their body must omit curly braces. Make
> reftable/pq.c conform to this guideline. Besides that, remove
> unnecessary newlines and variable assignment.
>
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
> reftable/pq.c | 18 ++++--------------
> 1 file changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/reftable/pq.c b/reftable/pq.c
> index 7fb45d8c60..0401c47068 100644
> --- a/reftable/pq.c
> +++ b/reftable/pq.c
> @@ -27,22 +27,16 @@ struct pq_entry merged_iter_pqueue_remove(struct merged_iter_pqueue *pq)
> pq->heap[0] = pq->heap[pq->len - 1];
> pq->len--;
>
> - i = 0;
> while (i < pq->len) {
> int min = i;
> int j = 2 * i + 1;
> int k = 2 * i + 2;
> - if (j < pq->len && pq_less(&pq->heap[j], &pq->heap[i])) {
> + if (j < pq->len && pq_less(&pq->heap[j], &pq->heap[i]))
> min = j;
> - }
> - if (k < pq->len && pq_less(&pq->heap[k], &pq->heap[min])) {
> + if (k < pq->len && pq_less(&pq->heap[k], &pq->heap[min]))
> min = k;
> - }
> -
> - if (min == i) {
> + if (min == i)
> break;
> - }
> -
> SWAP(pq->heap[i], pq->heap[min]);
> i = min;
> }
> @@ -53,19 +47,15 @@ struct pq_entry merged_iter_pqueue_remove(struct merged_iter_pqueue *pq)
> void merged_iter_pqueue_add(struct merged_iter_pqueue *pq, const struct pq_entry *e)
> {
> int i = 0;
> -Nit: I think this newline is helpful as it delimits the variable declarations from the actual code.
I wonder whether we should also change the type of `i` to `size_t` while at it, as `pq->len` is of type `size_t, as well (and further up in the other test). It does mean that we have to add an explicit check whether `pq->len == 0`, but that isn't all that bad.
In any case, if we want to do such a change, it should probably be in a separate commit.
Show 6 quoted lines
> REFTABLE_ALLOC_GROW(pq->heap, pq->len + 1, pq->cap);
> pq->heap[pq->len++] = *e;
>
> i = pq->len - 1;
> while (i > 0) {
> int j = (i - 1) / 2;The type of `j` is wrong, as well.
Other than that this patch series looks good to me, thanks.
Patrick