Re: [GSoC][PATCH v2 1/6] reftable: clean up reftable/pq.c

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2387 bytes --]

On Thu, Jun 06, 2024 at 08:53:37PM +0530, Chandra Pratap wrote:
> 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.

>  	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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]