Re: [GSoC Patch 2/3] commit: convert commit->generation to a slab

From: Derrick Stolee <stolee@gmail.com>
Date: Jun 4, 2020, 14:27 UTC
Message-ID: <9a15c7ba-8b55-099a-3c59-b5e7ff6124f6@gmail.com>
In-Reply-To: <20200604072759.19142-3-abhishekkumar8222@gmail.com>

On 6/4/2020 3:27 AM, Abhishek Kumar wrote:

Show 9 quoted lines

> In this commit, we will use the generation slab helpers introduced in
> last commit and replace existing uses of commit->generation using
> 'contrib/coccinelle/generation.cocci'
> @@ -1048,7 +1048,7 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,
>  		else
>  			packedDate[0] = 0;
>  
> -		packedDate[0] |= htonl((*list)->generation << 2);
> +		packedDate[0] |= htonl(generation((*list)) << 2);

nit: We no longer need the extra parens around *list.

Show 8 quoted lines

> @@ -3414,8 +3414,8 @@ static void init_topo_walk(struct rev_info *revs)
>  		test_flag_and_insert(&info->explore_queue, c, TOPO_WALK_EXPLORED);
>  		test_flag_and_insert(&info->indegree_queue, c, TOPO_WALK_INDEGREE);
>  
> -		if (c->generation < info->min_generation)
> -			info->min_generation = c->generation;
> +		if (generation(c) < info->min_generation)
> +			info->min_generation = generation(c);

A pattern I've noticed in several places is that the struct member is accessed multiple times in the same method body, and this is auto-converted to multiple method calls. However, these values are fixed, so it would be better to store the value as a local variable and reuse that variable instead.

This is one of the shortcomings of the Coccinelle transformation, so you'll need to manually inspect each of the diff fragments to see if we can reduce the number of method calls. It might be helpful to do that as a follow-up, so we can see that this patch is generated by the Coccinelle script, and then a later patch can be scrutinized more carefully when you are doing manual code manipulation.

Thanks, -Stolee