[PATCH] describe: use strbuf_add_unique_abbrev() for adding short hashes

[PATCH] describe: use strbuf_add_unique_abbrev() for adding short hashes

[René Scharfe]

Call strbuf_add_unique_abbrev() to add an abbreviated hash to a strbuf
instead of taking a detour through find_unique_abbrev() and its static
buffer.  This is shorter and a bit more efficient.

Patch generated by Coccinelle (and contrib/coccinelle/strbuf.cocci).

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
The changed line was added by 4dbc59a4cc (builtin/describe.c: factor
out describe_commit).

"make coccicheck" doesn't propose any other changes for current master.

 builtin/describe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 3b0b204b1e..21e37f5dae 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -380,7 +380,7 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst)
 	if (!match_cnt) {
 		struct object_id *cmit_oid = &cmit->object.oid;
 		if (always) {
-			strbuf_addstr(dst, find_unique_abbrev(cmit_oid->hash, abbrev));
+			strbuf_add_unique_abbrev(dst, cmit_oid->hash, abbrev);
 			if (suffix)
 				strbuf_addstr(dst, suffix);
 			return;
-- 
2.15.1

Re: [PATCH] describe: use strbuf_add_unique_abbrev() for adding short hashes

[Derrick Stolee]

On 1/15/2018 12:10 PM, René Scharfe wrote:
> Call strbuf_add_unique_abbrev() to add an abbreviated hash to a strbuf
> instead of taking a detour through find_unique_abbrev() and its static
> buffer.  This is shorter and a bit more efficient.
>
> Patch generated by Coccinelle (and contrib/coccinelle/strbuf.cocci).
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> The changed line was added by 4dbc59a4cc (builtin/describe.c: factor
> out describe_commit).
>
> "make coccicheck" doesn't propose any other changes for current master.
>
>   builtin/describe.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/describe.c b/builtin/describe.c
> index 3b0b204b1e..21e37f5dae 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -380,7 +380,7 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst)
>   	if (!match_cnt) {
>   		struct object_id *cmit_oid = &cmit->object.oid;
>   		if (always) {
> -			strbuf_addstr(dst, find_unique_abbrev(cmit_oid->hash, abbrev));
> +			strbuf_add_unique_abbrev(dst, cmit_oid->hash, abbrev);
>   			if (suffix)
>   				strbuf_addstr(dst, suffix);
>   			return;

René,

Thanks for this cleanup! I just learned about strbuf_add_unique_abbrev() 
and like that it uses the reentrant find_unique_abbrev_r() instead.

Looks good to me.
-Stolee

Re: [PATCH] describe: use strbuf_add_unique_abbrev() for adding short hashes

[SZEDER Gábor]

> Patch generated by Coccinelle (and contrib/coccinelle/strbuf.cocci).

Interesting.

The static analysis build job on Travis CI runs 'make coccicheck', so
it should have caught this.  However, I've looked at more build job
results than I could count while working on some Travis CI related
patches in the last few weeks, but I've never noticed Coccinelle
proposing anything.

Now I see that Coccinelle's, and in turn 'make coccicheck's exit code
only indicates that Coccinelle managed to finish without any errors
(e.g. no wrong --options, missing files or whatnot), but it has
nothing to do with whether it found something to transform or not.  To
find out the latter, we have to check the resulting
'contrib/coccinelle/*.cocci.patch' files or look closer at 'make
coccicheck's standard output.  If any of those '*.cocci.patch' files
are not empty and make's output contains lines like:

  SPATCH result: contrib/coccinelle/strbuf.cocci.patch

then Coccinelle found something.

Well, OK, I think that might be an acceptable behavior for developers
running 'make coccicheck' themselves, but totally unsuitable for
automated Travis CI build jobs, because it doesn't draw our attention
to Coccinelle's findings.  And the only means to draw attention in
an automated setting is to fail the buid job.

Good, I quickly whipped up a patch to fail the build job if any of the
resulting '*.cocci.patch' files are not empty and also to include
their content in the trace log, to see how that would work out, and...

> "make coccicheck" doesn't propose any other changes for current master.

... and found that 'make coccicheck' on Travis CI _does_ propose
further changes for current master.  You can find the build job's
output including those proposed changes here:

  https://travis-ci.org/szeder/git/jobs/329296813#L586

The proposed changes coming from 'array.cocci' are replacing memmove()
calls with MOVE_ARRAY().  After a (very) cursory look they all seem to
make sense to me; at least after applying those changes Git can be
built and the test suite still succeeds.

Unfortunately, most of the changes coming from 'strbuf.cocci' don't
make any sense, they appear to be the mis-application of the "use
strbuf_addstr() instead of strbuf_addf() to add a single string" rule:

  -             strbuf_addf(&sb_repo, "%d", counter);
  +             strbuf_addstr(&sb_repo, counter);

It seems that those rules need some refinement, but I have no idea
about Coccinelle and this is not the time for me to dig deeper.

What makes all this weird is that running 'make coccicheck' on my own
machine doesn't produce any of these additional proposed changes, just
like at René's.  Can it be related to differing Coccinelle versions?
Travis CI installs 1.0.0~rc19.deb-3; I have 1.0.4.deb-2.


Gábor

Re: [PATCH] describe: use strbuf_add_unique_abbrev() for adding short hashes

[René Scharfe]

Am 16.01.2018 um 18:11 schrieb SZEDER Gábor:
> Unfortunately, most of the changes coming from 'strbuf.cocci' don't
> make any sense, they appear to be the mis-application of the "use
> strbuf_addstr() instead of strbuf_addf() to add a single string" rule:
> 
>    -             strbuf_addf(&sb_repo, "%d", counter);
>    +             strbuf_addstr(&sb_repo, counter);
> 
> It seems that those rules need some refinement, but I have no idea
> about Coccinelle and this is not the time for me to dig deeper.
> 
> What makes all this weird is that running 'make coccicheck' on my own
> machine doesn't produce any of these additional proposed changes, just
> like at René's.  Can it be related to differing Coccinelle versions?
> Travis CI installs 1.0.0~rc19.deb-3; I have 1.0.4.deb-2.

The version difference may explain it, but I couldn't find a matching
bugfix in http://coccinelle.lip6.fr/distrib/changes.html when I just
skimmed it.  I wonder if the following patch could make a difference:

---
 contrib/coccinelle/strbuf.cocci | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci
index 1d580e49b0..6fe8727421 100644
--- a/contrib/coccinelle/strbuf.cocci
+++ b/contrib/coccinelle/strbuf.cocci
@@ -29,8 +29,9 @@ cocci.include_match("%" not in fmt)
 
 @@
 expression E1, E2;
+format F =~ "s";
 @@
-- strbuf_addf(E1, "%s", E2);
+- strbuf_addf(E1, "%@F@", E2);
 + strbuf_addstr(E1, E2);
 
 @@
-- 
2.16.0

Re: [PATCH] describe: use strbuf_add_unique_abbrev() for adding short hashes

[SZEDER Gábor]

On Thu, Jan 18, 2018 at 10:40 PM, René Scharfe <l.s.r@web.de> wrote:
> Am 16.01.2018 um 18:11 schrieb SZEDER Gábor:
>> Unfortunately, most of the changes coming from 'strbuf.cocci' don't
>> make any sense, they appear to be the mis-application of the "use
>> strbuf_addstr() instead of strbuf_addf() to add a single string" rule:
>>
>>    -             strbuf_addf(&sb_repo, "%d", counter);
>>    +             strbuf_addstr(&sb_repo, counter);
>>
>> It seems that those rules need some refinement, but I have no idea
>> about Coccinelle and this is not the time for me to dig deeper.
>>
>> What makes all this weird is that running 'make coccicheck' on my own
>> machine doesn't produce any of these additional proposed changes, just
>> like at René's.  Can it be related to differing Coccinelle versions?
>> Travis CI installs 1.0.0~rc19.deb-3; I have 1.0.4.deb-2.
>
> The version difference may explain it, but I couldn't find a matching
> bugfix in http://coccinelle.lip6.fr/distrib/changes.html when I just
> skimmed it.  I wonder if the following patch could make a difference:

Yes, it does, now all those nonsense suggestions are gone on Travis CI.

  https://travis-ci.org/szeder/git/jobs/330572425#L713

Those "memmove() -> MOVE_ARRAY" suggestions are still there, of course.

> ---
>  contrib/coccinelle/strbuf.cocci | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci
> index 1d580e49b0..6fe8727421 100644
> --- a/contrib/coccinelle/strbuf.cocci
> +++ b/contrib/coccinelle/strbuf.cocci
> @@ -29,8 +29,9 @@ cocci.include_match("%" not in fmt)
>
>  @@
>  expression E1, E2;
> +format F =~ "s";
>  @@
> -- strbuf_addf(E1, "%s", E2);
> +- strbuf_addf(E1, "%@F@", E2);
>  + strbuf_addstr(E1, E2);
>
>  @@
> --
> 2.16.0

Re: [PATCH] describe: use strbuf_add_unique_abbrev() for adding short hashes

[Lars Schneider]

> On 18 Jan 2018, at 23:40, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> 
> On Thu, Jan 18, 2018 at 10:40 PM, René Scharfe <l.s.r@web.de> wrote:
>> Am 16.01.2018 um 18:11 schrieb SZEDER Gábor:
>>> Unfortunately, most of the changes coming from 'strbuf.cocci' don't
>>> make any sense, they appear to be the mis-application of the "use
>>> strbuf_addstr() instead of strbuf_addf() to add a single string" rule:
>>> 
>>>   -             strbuf_addf(&sb_repo, "%d", counter);
>>>   +             strbuf_addstr(&sb_repo, counter);
>>> 
>>> It seems that those rules need some refinement, but I have no idea
>>> about Coccinelle and this is not the time for me to dig deeper.
>>> 
>>> What makes all this weird is that running 'make coccicheck' on my own
>>> machine doesn't produce any of these additional proposed changes, just
>>> like at René's.  Can it be related to differing Coccinelle versions?
>>> Travis CI installs 1.0.0~rc19.deb-3; I have 1.0.4.deb-2.
>> 
>> The version difference may explain it, but I couldn't find a matching
>> bugfix in http://coccinelle.lip6.fr/distrib/changes.html when I just
>> skimmed it.  I wonder if the following patch could make a difference:
> 
> Yes, it does, now all those nonsense suggestions are gone on Travis CI.
> 
>  https://travis-ci.org/szeder/git/jobs/330572425#L713
> 
> Those "memmove() -> MOVE_ARRAY" suggestions are still there, of course.

Nice!

Travis CI runs Ubuntu Trusty as current base image with Coccinelle
version "1.0.0~rc19.deb-3": https://packages.ubuntu.com/trusty/coccinelle

I was experimenting with a docker container that has the latest stable
version of Coccinelle installed ("1.0.4.deb-2"). We could run this
docker container inside of the Travis CI job similar to the 32-bit job.
But with René's changes that doesn't seem to be necessary anymore!

- Lars


PS: Thanks a lot Gábor for recognizing and fixing the invalid
    Travis CI Coccinelle build!

Re: [PATCH] describe: use strbuf_add_unique_abbrev() for adding short hashes

[René Scharfe]

Am 18.01.2018 um 23:40 schrieb SZEDER Gábor:
> On Thu, Jan 18, 2018 at 10:40 PM, René Scharfe <l.s.r@web.de> wrote:
>> Am 16.01.2018 um 18:11 schrieb SZEDER Gábor:
>>> Unfortunately, most of the changes coming from 'strbuf.cocci' don't
>>> make any sense, they appear to be the mis-application of the "use
>>> strbuf_addstr() instead of strbuf_addf() to add a single string" rule:
>>>
>>>     -             strbuf_addf(&sb_repo, "%d", counter);
>>>     +             strbuf_addstr(&sb_repo, counter);
>>>
>>> It seems that those rules need some refinement, but I have no idea
>>> about Coccinelle and this is not the time for me to dig deeper.
>>>
>>> What makes all this weird is that running 'make coccicheck' on my own
>>> machine doesn't produce any of these additional proposed changes, just
>>> like at René's.  Can it be related to differing Coccinelle versions?
>>> Travis CI installs 1.0.0~rc19.deb-3; I have 1.0.4.deb-2.
>>
>> The version difference may explain it, but I couldn't find a matching
>> bugfix in http://coccinelle.lip6.fr/distrib/changes.html when I just
>> skimmed it.  I wonder if the following patch could make a difference:
> 
> Yes, it does, now all those nonsense suggestions are gone on Travis CI.

I would have expected matching a literal "%s" to be easier than
dissecting that (admittedly simple) format string, but if it all works
out fine then I'm not complaining. :)  Sent the patch again properly.

>    https://travis-ci.org/szeder/git/jobs/330572425#L713
> 
> Those "memmove() -> MOVE_ARRAY" suggestions are still there, of course.

They look valid and nice to have in that report.  I wonder why we don't
get them locally, though.  Are you going to submit them as a patch?

(NB: The patches generated by coccicheck apply with "patch -p0", unlike
 those generated by git diff and friends.)

Thanks,
René

[PATCH] Use MOVE_ARRAY

[SZEDER Gábor]

Use the helper macro MOVE_ARRAY to move arrays.  This is shorter and
safer, as it automatically infers the size of elements.

Patch generated by Coccinelle and contrib/coccinelle/array.cocci in
Travis CI's static analysis build job.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 cache-tree.c      | 5 ++---
 commit.c          | 6 ++----
 diffcore-rename.c | 8 ++++----
 dir.c             | 4 ++--
 parse-options.c   | 2 +-
 read-cache.c      | 5 ++---
 refs/ref-cache.c  | 6 ++----
 replace_object.c  | 6 ++----
 rerere.c          | 4 ++--
 9 files changed, 19 insertions(+), 27 deletions(-)

diff --git a/cache-tree.c b/cache-tree.c
index e03e72c34a..18946aa458 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -84,9 +84,8 @@ static struct cache_tree_sub *find_subtree(struct cache_tree *it,
 	down->namelen = pathlen;
 
 	if (pos < it->subtree_nr)
-		memmove(it->down + pos + 1,
-			it->down + pos,
-			sizeof(down) * (it->subtree_nr - pos - 1));
+		MOVE_ARRAY(it->down + pos + 1, it->down + pos,
+			   it->subtree_nr - pos - 1);
 	it->down[pos] = down;
 	return down;
 }
diff --git a/commit.c b/commit.c
index cab8d4455b..7d0080180d 100644
--- a/commit.c
+++ b/commit.c
@@ -126,10 +126,8 @@ int register_commit_graft(struct commit_graft *graft, int ignore_dups)
 	ALLOC_GROW(commit_graft, commit_graft_nr + 1, commit_graft_alloc);
 	commit_graft_nr++;
 	if (pos < commit_graft_nr)
-		memmove(commit_graft + pos + 1,
-			commit_graft + pos,
-			(commit_graft_nr - pos - 1) *
-			sizeof(*commit_graft));
+		MOVE_ARRAY(commit_graft + pos + 1, commit_graft + pos,
+			   commit_graft_nr - pos - 1);
 	commit_graft[pos] = graft;
 	return 0;
 }
diff --git a/diffcore-rename.c b/diffcore-rename.c
index 245e999fe5..888a4b0189 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -57,8 +57,8 @@ static int add_rename_dst(struct diff_filespec *two)
 	ALLOC_GROW(rename_dst, rename_dst_nr + 1, rename_dst_alloc);
 	rename_dst_nr++;
 	if (first < rename_dst_nr)
-		memmove(rename_dst + first + 1, rename_dst + first,
-			(rename_dst_nr - first - 1) * sizeof(*rename_dst));
+		MOVE_ARRAY(rename_dst + first + 1, rename_dst + first,
+			   rename_dst_nr - first - 1);
 	rename_dst[first].two = alloc_filespec(two->path);
 	fill_filespec(rename_dst[first].two, &two->oid, two->oid_valid,
 		      two->mode);
@@ -98,8 +98,8 @@ static struct diff_rename_src *register_rename_src(struct diff_filepair *p)
 	ALLOC_GROW(rename_src, rename_src_nr + 1, rename_src_alloc);
 	rename_src_nr++;
 	if (first < rename_src_nr)
-		memmove(rename_src + first + 1, rename_src + first,
-			(rename_src_nr - first - 1) * sizeof(*rename_src));
+		MOVE_ARRAY(rename_src + first + 1, rename_src + first,
+			   rename_src_nr - first - 1);
 	rename_src[first].p = p;
 	rename_src[first].score = score;
 	return &(rename_src[first]);
diff --git a/dir.c b/dir.c
index 7c4b45e30e..ce6e50d2a2 100644
--- a/dir.c
+++ b/dir.c
@@ -747,8 +747,8 @@ static struct untracked_cache_dir *lookup_untracked(struct untracked_cache *uc,
 	FLEX_ALLOC_MEM(d, name, name, len);
 
 	ALLOC_GROW(dir->dirs, dir->dirs_nr + 1, dir->dirs_alloc);
-	memmove(dir->dirs + first + 1, dir->dirs + first,
-		(dir->dirs_nr - first) * sizeof(*dir->dirs));
+	MOVE_ARRAY(dir->dirs + first + 1, dir->dirs + first,
+		   dir->dirs_nr - first);
 	dir->dirs_nr++;
 	dir->dirs[first] = d;
 	return d;
diff --git a/parse-options.c b/parse-options.c
index fca7159646..d02eb8b015 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -525,7 +525,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 
 int parse_options_end(struct parse_opt_ctx_t *ctx)
 {
-	memmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * sizeof(*ctx->out));
+	MOVE_ARRAY(ctx->out + ctx->cpidx, ctx->argv, ctx->argc);
 	ctx->out[ctx->cpidx + ctx->argc] = NULL;
 	return ctx->cpidx + ctx->argc;
 }
diff --git a/read-cache.c b/read-cache.c
index 2eb81a66b9..2e8c85c686 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1217,9 +1217,8 @@ int add_index_entry(struct index_state *istate, struct cache_entry *ce, int opti
 	/* Add it in.. */
 	istate->cache_nr++;
 	if (istate->cache_nr > pos + 1)
-		memmove(istate->cache + pos + 1,
-			istate->cache + pos,
-			(istate->cache_nr - pos - 1) * sizeof(ce));
+		MOVE_ARRAY(istate->cache + pos + 1, istate->cache + pos,
+			   istate->cache_nr - pos - 1);
 	set_index_entry(istate, pos, ce);
 	istate->cache_changed |= CE_ENTRY_ADDED;
 	return 0;
diff --git a/refs/ref-cache.c b/refs/ref-cache.c
index 82c1cf90a7..e90bd3e727 100644
--- a/refs/ref-cache.c
+++ b/refs/ref-cache.c
@@ -238,10 +238,8 @@ int remove_entry_from_dir(struct ref_dir *dir, const char *refname)
 		return -1;
 	entry = dir->entries[entry_index];
 
-	memmove(&dir->entries[entry_index],
-		&dir->entries[entry_index + 1],
-		(dir->nr - entry_index - 1) * sizeof(*dir->entries)
-		);
+	MOVE_ARRAY(&dir->entries[entry_index],
+		   &dir->entries[entry_index + 1], dir->nr - entry_index - 1);
 	dir->nr--;
 	if (dir->sorted > entry_index)
 		dir->sorted--;
diff --git a/replace_object.c b/replace_object.c
index f0b39f06d5..3e49965d05 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -44,10 +44,8 @@ static int register_replace_object(struct replace_object *replace,
 	ALLOC_GROW(replace_object, replace_object_nr + 1, replace_object_alloc);
 	replace_object_nr++;
 	if (pos < replace_object_nr)
-		memmove(replace_object + pos + 1,
-			replace_object + pos,
-			(replace_object_nr - pos - 1) *
-			sizeof(*replace_object));
+		MOVE_ARRAY(replace_object + pos + 1, replace_object + pos,
+			   replace_object_nr - pos - 1);
 	replace_object[pos] = replace;
 	return 0;
 }
diff --git a/rerere.c b/rerere.c
index 1ce440f4bb..79203c6c1e 100644
--- a/rerere.c
+++ b/rerere.c
@@ -159,8 +159,8 @@ static struct rerere_dir *find_rerere_dir(const char *hex)
 		ALLOC_GROW(rerere_dir, rerere_dir_nr + 1, rerere_dir_alloc);
 		/* ... and add it in. */
 		rerere_dir_nr++;
-		memmove(rerere_dir + pos + 1, rerere_dir + pos,
-			(rerere_dir_nr - pos - 1) * sizeof(*rerere_dir));
+		MOVE_ARRAY(rerere_dir + pos + 1, rerere_dir + pos,
+			   rerere_dir_nr - pos - 1);
 		rerere_dir[pos] = rr_dir;
 		scan_rerere_dir(rr_dir);
 	}
-- 
2.16.1.80.gc0eec9753d

Re: [PATCH] Use MOVE_ARRAY

[Jeff King]

On Mon, Jan 22, 2018 at 06:50:09PM +0100, SZEDER Gábor wrote:

> Use the helper macro MOVE_ARRAY to move arrays.  This is shorter and
> safer, as it automatically infers the size of elements.
> 
> Patch generated by Coccinelle and contrib/coccinelle/array.cocci in
> Travis CI's static analysis build job.

Seems pretty straightforward. One thing I did notice...

> diff --git a/cache-tree.c b/cache-tree.c
> index e03e72c34a..18946aa458 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -84,9 +84,8 @@ static struct cache_tree_sub *find_subtree(struct cache_tree *it,
>  	down->namelen = pathlen;
>  
>  	if (pos < it->subtree_nr)
> -		memmove(it->down + pos + 1,
> -			it->down + pos,
> -			sizeof(down) * (it->subtree_nr - pos - 1));
> +		MOVE_ARRAY(it->down + pos + 1, it->down + pos,
> +			   it->subtree_nr - pos - 1);

Most of these are "shift part of the array". I wonder if it would make
sense to encapsulate that pattern in a helper, like:

  #define SHIFT_ARRAY(a, nr, pos, slots) \
    MOVE_ARRAY(a + pos + slots, a + pos, nr - pos - slots)
  ...
  SHIFT_ARRAY(it->down, it->subtree_nr, pos, 1);

I'm not sure if that's more readable because it describes a higher-level
operation, or if it's less because it adds yet another non-standard
helper for the reader to learn.

-Peff

Re: [PATCH] Use MOVE_ARRAY

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Most of these are "shift part of the array". I wonder if it would make
> sense to encapsulate that pattern in a helper, like:
>
>   #define SHIFT_ARRAY(a, nr, pos, slots) \
>     MOVE_ARRAY(a + pos + slots, a + pos, nr - pos - slots)
>   ...
>   SHIFT_ARRAY(it->down, it->subtree_nr, pos, 1);

Exactly my thought when I was queuing it, but I was wondering about
this more from "can we use the higher level abstraction for reducing
errors?" point of view.  If we are shifting an array by 3 slots to
the right, we should at least have enough slots allocated to hold
them (i.e. a->nr - a->alloc must be 3 or more).  But after realizing
that the level these macros operate at is still a bit too low to do
something like that, I quickly lost interest ;-)

> I'm not sure if that's more readable because it describes a higher-level
> operation, or if it's less because it adds yet another non-standard
> helper for the reader to learn.

Yeah, conflicting goals.  It indeed is easier to see what is going
on when reading the code, once the reader gets used to them.

Thanks.

Re: [PATCH] Use MOVE_ARRAY

[Jeff King]

On Mon, Jan 22, 2018 at 03:26:59PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Most of these are "shift part of the array". I wonder if it would make
> > sense to encapsulate that pattern in a helper, like:
> >
> >   #define SHIFT_ARRAY(a, nr, pos, slots) \
> >     MOVE_ARRAY(a + pos + slots, a + pos, nr - pos - slots)
> >   ...
> >   SHIFT_ARRAY(it->down, it->subtree_nr, pos, 1);
> 
> Exactly my thought when I was queuing it, but I was wondering about
> this more from "can we use the higher level abstraction for reducing
> errors?" point of view.  If we are shifting an array by 3 slots to
> the right, we should at least have enough slots allocated to hold
> them (i.e. a->nr - a->alloc must be 3 or more).  But after realizing
> that the level these macros operate at is still a bit too low to do
> something like that, I quickly lost interest ;-)

Yeah, you'd need to know the "alloc" number to right-shift correctly,
since by definition we're pushing off the end of a->nr. Left-shifts just
need to make sure they don't go past "0", which we can do here, but I'd
think they're pretty uncommon.

The right macro level may actually be something more like "make room for
N items at pos". E.g.:

  #define CREATE_ARRAY_HOLE(a, nr, alloc, pos, slots) do { \
          ALLOC_GROW(a, nr + slots, alloc);
	  MOVE_ARRAY(a + pos + slots, a + pos, nr - pos - slots);
  } while (0)

but I didn't investigate the surrounding code. And it surely would need
a catchier name. ;)

-Peff