threads / patch / 51886

patchgit: use COPY_ARRAY and MOVE_ARRAY in handle_alias()

Subject: [PATCH] git: use COPY_ARRAY and MOVE_ARRAY in handle_alias()

## tl;dr

7 messages between Sep 19, 2019 and Sep 26, 2019. Diffs are folded; open one to read it.

replies: 6people: 6as markdown or json

René Scharfe· Sep 19, 2019, 20:48 UTC · lore

Use the macro COPY_ARRAY to copy array elements and MOVE_ARRAY to do the same for moving them backwards in an array with potential overlap. The result is shorter and safer, as it infers the element type automatically and does a (very) basic type compatibility check for its first two arguments.

These cases were missed by Coccinelle and contrib/coccinelle/array.cocci because the type of the elements is "const char *", not "char *", and the rules in the semantic patch cautiously insist on the sizeof operator being used on exactly the same type to avoid generating transformations that introduce subtle bugs into tricky code.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 git.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
Show changes to git.c +2 −4
diff --git a/git.c b/git.c
index c1ee7124ed..ce6ab0ece2 100644
--- a/git.c
+++ b/git.c
@@ -369,8 +369,7 @@ static int handle_alias(int *argcp, const char ***argv)
 			die(_("alias '%s' changes environment variables.\n"
 			      "You can use '!git' in the alias to do this"),
 			    alias_command);
-		memmove(new_argv - option_count, new_argv,
-				count * sizeof(char *));
+		MOVE_ARRAY(new_argv - option_count, new_argv, count);
 		new_argv -= option_count;

 		if (count < 1)
@@ -385,7 +384,7 @@ static int handle_alias(int *argcp, const char ***argv)

 		REALLOC_ARRAY(new_argv, count + *argcp);
 		/* insert after command name */
-		memcpy(new_argv + count, *argv + 1, sizeof(char *) * *argcp);
+		COPY_ARRAY(new_argv + count, *argv + 1, *argcp);

 		trace2_cmd_alias(alias_command, new_argv);
 		trace2_cmd_list_config();
--
2.23.0
Jeff King· Sep 23, 2019, 22:27 UTC · re: René Scharfe · lore

Re: [PATCH] git: use COPY_ARRAY and MOVE_ARRAY in handle_alias()

On Thu, Sep 19, 2019 at 10:48:30PM +0200, René Scharfe wrote:
Show 11 quoted lines
> Use the macro COPY_ARRAY to copy array elements and MOVE_ARRAY to do the
> same for moving them backwards in an array with potential overlap.  The
> result is shorter and safer, as it infers the element type automatically
> and does a (very) basic type compatibility check for its first two
> arguments.
> 
> These cases were missed by Coccinelle and contrib/coccinelle/array.cocci
> because the type of the elements is "const char *", not "char *", and
> the rules in the semantic patch cautiously insist on the sizeof operator
> being used on exactly the same type to avoid generating transformations
> that introduce subtle bugs into tricky code.
Another good reason to use "sizeof(var)" instead of sizeof(type)". :)
The patch looks good.
-Peff
Johannes Schindelin· Sep 26, 2019, 13:22 UTC · re: Jeff King · lore

sizeof(var) vs sizeof(type), was Re: [PATCH] git: use COPY_ARRAY and MOVE_ARRAY in handle_alias()

Hi Peff,
On Mon, 23 Sep 2019, Jeff King wrote:
Show 15 quoted lines
> On Thu, Sep 19, 2019 at 10:48:30PM +0200, René Scharfe wrote:
>
> > Use the macro COPY_ARRAY to copy array elements and MOVE_ARRAY to do the
> > same for moving them backwards in an array with potential overlap.  The
> > result is shorter and safer, as it infers the element type automatically
> > and does a (very) basic type compatibility check for its first two
> > arguments.
> >
> > These cases were missed by Coccinelle and contrib/coccinelle/array.cocci
> > because the type of the elements is "const char *", not "char *", and
> > the rules in the semantic patch cautiously insist on the sizeof operator
> > being used on exactly the same type to avoid generating transformations
> > that introduce subtle bugs into tricky code.
>
> Another good reason to use "sizeof(var)" instead of sizeof(type)". :)

That is indeed a very good reason, in addition to getting the type right automatically (by virtue of letting the compiler pick it).

Should we make this an explicit guideline in our documentation?

Ciao, Dscho

Derrick Stolee· Sep 26, 2019, 13:36 UTC · re: Johannes Schindelin · lore

Re: sizeof(var) vs sizeof(type), was Re: [PATCH] git: use COPY_ARRAY and MOVE_ARRAY in handle_alias()

On 9/26/2019 9:22 AM, Johannes Schindelin wrote:
Show 24 quoted lines
> Hi Peff,
> 
> On Mon, 23 Sep 2019, Jeff King wrote:
> 
>> On Thu, Sep 19, 2019 at 10:48:30PM +0200, René Scharfe wrote:
>>
>>> Use the macro COPY_ARRAY to copy array elements and MOVE_ARRAY to do the
>>> same for moving them backwards in an array with potential overlap.  The
>>> result is shorter and safer, as it infers the element type automatically
>>> and does a (very) basic type compatibility check for its first two
>>> arguments.
>>>
>>> These cases were missed by Coccinelle and contrib/coccinelle/array.cocci
>>> because the type of the elements is "const char *", not "char *", and
>>> the rules in the semantic patch cautiously insist on the sizeof operator
>>> being used on exactly the same type to avoid generating transformations
>>> that introduce subtle bugs into tricky code.
>>
>> Another good reason to use "sizeof(var)" instead of sizeof(type)". :)
> 
> That is indeed a very good reason, in addition to getting the type right
> automatically (by virtue of letting the compiler pick it).
> 
> Should we make this an explicit guideline in our documentation?
Better yet: can we create a Coccinelle script to fix it automatically?
-Stolee
SZEDER Gábor· Sep 26, 2019, 13:43 UTC · re: Derrick Stolee · lore

Re: sizeof(var) vs sizeof(type), was Re: [PATCH] git: use COPY_ARRAY and MOVE_ARRAY in handle_alias()

On Thu, Sep 26, 2019 at 09:36:44AM -0400, Derrick Stolee wrote:
Show 27 quoted lines
> On 9/26/2019 9:22 AM, Johannes Schindelin wrote:
> > Hi Peff,
> > 
> > On Mon, 23 Sep 2019, Jeff King wrote:
> > 
> >> On Thu, Sep 19, 2019 at 10:48:30PM +0200, René Scharfe wrote:
> >>
> >>> Use the macro COPY_ARRAY to copy array elements and MOVE_ARRAY to do the
> >>> same for moving them backwards in an array with potential overlap.  The
> >>> result is shorter and safer, as it infers the element type automatically
> >>> and does a (very) basic type compatibility check for its first two
> >>> arguments.
> >>>
> >>> These cases were missed by Coccinelle and contrib/coccinelle/array.cocci
> >>> because the type of the elements is "const char *", not "char *", and
> >>> the rules in the semantic patch cautiously insist on the sizeof operator
> >>> being used on exactly the same type to avoid generating transformations
> >>> that introduce subtle bugs into tricky code.
> >>
> >> Another good reason to use "sizeof(var)" instead of sizeof(type)". :)
> > 
> > That is indeed a very good reason, in addition to getting the type right
> > automatically (by virtue of letting the compiler pick it).
> > 
> > Should we make this an explicit guideline in our documentation?
> 
> Better yet: can we create a Coccinelle script to fix it automatically?

I've already done that well over a year ago :) But remember not being quite satisfied with something (no idea what it was anymore) and left it on the backburner.

Will dig it out and have a look as time permits.
Philip Oakley· Sep 26, 2019, 15:24 UTC · re: Derrick Stolee · lore

Re: sizeof(var) vs sizeof(type), was Re: [PATCH] git: use COPY_ARRAY and MOVE_ARRAY in handle_alias()

On 26/09/2019 14:36, Derrick Stolee wrote:
Show 9 quoted lines
>>> Another good reason to use "sizeof(var)" instead of sizeof(type)". :)
>> That is indeed a very good reason, in addition to getting the type right
>> automatically (by virtue of letting the compiler pick it).
>>
>> Should we make this an explicit guideline in our documentation?
> Better yet: can we create a Coccinelle script to fix it automatically?
>
> -Stolee
>

How about 'Both'. We can't assume all contributors have Coccinelle on their OS/system.

Philip
Derrick Stolee· Sep 26, 2019, 16:16 UTC · re: Philip Oakley · lore

Re: sizeof(var) vs sizeof(type), was Re: [PATCH] git: use COPY_ARRAY and MOVE_ARRAY in handle_alias()

On 9/26/2019 11:24 AM, Philip Oakley wrote:
Show 11 quoted lines
> On 26/09/2019 14:36, Derrick Stolee wrote:
>>>> Another good reason to use "sizeof(var)" instead of sizeof(type)". :)
>>> That is indeed a very good reason, in addition to getting the type right
>>> automatically (by virtue of letting the compiler pick it).
>>>
>>> Should we make this an explicit guideline in our documentation?
>> Better yet: can we create a Coccinelle script to fix it automatically?
>>
>> -Stolee
>>
> How about 'Both'. We can't assume all contributors have Coccinelle on their OS/system.

Both is best, but I find static checkers to be more reliable than updating documentation. For that reason, I would prioritize the Coccinelle script over adding another bullet point to the style guide.

The PR builds for GitGitGadget run ci/run-static-analysis.sh as a check (see the StaticAnalysis job in [1] for an example). That provides a free way to get feedback for users without Coccinelle.

[1] https://dev.azure.com/gitgitgadget/git/_build/results?buildId=16864&view=logs

Thanks, -Stolee

← back to recent threads