Re: [PATCH v1] Travis: also test on 32-bit Linux

[Jeff King <peff@peff.net>]

On Fri, Mar 10, 2017 at 01:14:16AM +0100, René Scharfe wrote:

> >   2. Ones which just copy a single object, like:
> > 
> >        memcpy(&dst, &src, sizeof(dst));
> > 
> >      Perhaps we should be using struct assignment like:
> > 
> >        dst = src;
> > 
> >      here. It's safer and it should give the compiler more room to
> >      optimize. The only downside is that if you have pointers, it is
> >      easy to write "dst = src" when you meant "*dst = *src".
> 
> Compilers can usually inline memcpy(3) calls, but assignments are
> shorter and more pleasing to the eye, and we get a type check for
> free.  How about this?

Yeah, I mostly wasn't sure how people felt about "shorter and more
pleasing". It _is_ shorter and there's less to get wrong. But the
memcpy() screams "hey, I am making a copy" and is idiomatic to at least
a certain generation of C programmers.

I guess something like COPY(dst, src) removes the part that you can get
wrong, while still screaming copy. It's not idiomatic either, but at
least it stands out. I dunno.

> diff --git a/contrib/coccinelle/copy.cocci b/contrib/coccinelle/copy.cocci
> new file mode 100644
> index 0000000000..f0d883932a
> --- /dev/null
> +++ b/contrib/coccinelle/copy.cocci
> @@ -0,0 +1,31 @@
> +@@
> +type T;
> +T dst;
> +T src;
> [...]
> +type T;
> +T dst;
> +T *src;

I think this misses the other two cases: (*dst, src) and (*dst, *src).

Perhaps squash this in?

---
 builtin/blame.c               |  2 +-
 builtin/pack-redundant.c      |  4 ++--
 contrib/coccinelle/copy.cocci | 32 ++++++++++++++++++++++++++++++++
 diff.c                        |  4 ++--
 dir.c                         |  2 +-
 fast-import.c                 |  6 +++---
 line-log.c                    |  2 +-
 7 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index f7aa95f4b..d0d917b37 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -680,7 +680,7 @@ static void dup_entry(struct blame_entry ***queue,
 {
 	origin_incref(src->suspect);
 	origin_decref(dst->suspect);
-	memcpy(dst, src, sizeof(*src));
+	*dst = *src;
 	dst->next = **queue;
 	**queue = dst;
 	*queue = &dst->next;
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 72c815844..ac1d8111e 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -207,7 +207,7 @@ static inline struct pack_list * pack_list_insert(struct pack_list **pl,
 					   struct pack_list *entry)
 {
 	struct pack_list *p = xmalloc(sizeof(struct pack_list));
-	memcpy(p, entry, sizeof(struct pack_list));
+	*p = *entry;
 	p->next = *pl;
 	*pl = p;
 	return p;
@@ -451,7 +451,7 @@ static void minimize(struct pack_list **min)
 		while (perm) {
 			if (is_superset(perm->pl, missing)) {
 				new_perm = xmalloc(sizeof(struct pll));
-				memcpy(new_perm, perm, sizeof(struct pll));
+				*new_perm = *perm;
 				new_perm->next = perm_ok;
 				perm_ok = new_perm;
 			}
diff --git a/contrib/coccinelle/copy.cocci b/contrib/coccinelle/copy.cocci
index f0d883932..da9969c84 100644
--- a/contrib/coccinelle/copy.cocci
+++ b/contrib/coccinelle/copy.cocci
@@ -29,3 +29,35 @@ T *src;
 - memcpy(&dst, src, sizeof(T));
 + dst = *src;
 )
+
+@@
+type T;
+T *dst;
+T src;
+@@
+(
+- memcpy(dst, &src, sizeof(*dst));
++ *dst = src;
+|
+- memcpy(dst, &src, sizeof(src));
++ *dst = src;
+|
+- memcpy(dst, &src, sizeof(T));
++ *dst = src;
+)
+
+@@
+type T;
+T *dst;
+T *src;
+@@
+(
+- memcpy(dst, src, sizeof(*dst));
++ *dst = *src;
+|
+- memcpy(dst, src, sizeof(*src));
++ *dst = *src;
+|
+- memcpy(dst, src, sizeof(T));
++ *dst = *src;
+)
diff --git a/diff.c b/diff.c
index 051761be4..fbd68ecd0 100644
--- a/diff.c
+++ b/diff.c
@@ -1169,7 +1169,7 @@ static void init_diff_words_data(struct emit_callback *ecbdata,
 {
 	int i;
 	struct diff_options *o = xmalloc(sizeof(struct diff_options));
-	memcpy(o, orig_opts, sizeof(struct diff_options));
+	*o = *orig_opts;
 
 	ecbdata->diff_words =
 		xcalloc(1, sizeof(struct diff_words_data));
@@ -3359,7 +3359,7 @@ static void run_checkdiff(struct diff_filepair *p, struct diff_options *o)
 
 void diff_setup(struct diff_options *options)
 {
-	memcpy(options, &default_diff_options, sizeof(*options));
+	*options = default_diff_options;
 
 	options->file = stdout;
 
diff --git a/dir.c b/dir.c
index 4541f9e14..d3d0039bf 100644
--- a/dir.c
+++ b/dir.c
@@ -2499,7 +2499,7 @@ static int read_one_dir(struct untracked_cache_dir **untracked_,
 	if (next > rd->end)
 		return -1;
 	*untracked_ = untracked = xmalloc(st_add(sizeof(*untracked), len));
-	memcpy(untracked, &ud, sizeof(ud));
+	*untracked = ud;
 	memcpy(untracked->name, data, len + 1);
 	data = next;
 
diff --git a/fast-import.c b/fast-import.c
index 6c13472c4..3e294c2b5 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -875,7 +875,7 @@ static struct tree_content *dup_tree_content(struct tree_content *s)
 	for (i = 0; i < s->entry_count; i++) {
 		a = s->entries[i];
 		b = new_tree_entry();
-		memcpy(b, a, sizeof(*a));
+		*b = *a;
 		if (a->tree && is_null_sha1(b->versions[1].sha1))
 			b->tree = dup_tree_content(a->tree);
 		else
@@ -1685,7 +1685,7 @@ static int tree_content_remove(
 
 del_entry:
 	if (backup_leaf)
-		memcpy(backup_leaf, e, sizeof(*backup_leaf));
+		*backup_leaf = *e;
 	else if (e->tree)
 		release_tree_content_recursive(e->tree);
 	e->tree = NULL;
@@ -1735,7 +1735,7 @@ static int tree_content_get(
 	return 0;
 
 found_entry:
-	memcpy(leaf, e, sizeof(*leaf));
+	*leaf = *e;
 	if (e->tree && is_null_sha1(e->versions[1].sha1))
 		leaf->tree = dup_tree_content(e->tree);
 	else
diff --git a/line-log.c b/line-log.c
index 64f141e20..de5bbb5bd 100644
--- a/line-log.c
+++ b/line-log.c
@@ -765,7 +765,7 @@ static void move_diff_queue(struct diff_queue_struct *dst,
 			    struct diff_queue_struct *src)
 {
 	assert(src != dst);
-	memcpy(dst, src, sizeof(struct diff_queue_struct));
+	*dst = *src;
 	DIFF_QUEUE_CLEAR(src);
 }