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

[Jeff King <peff@peff.net>]

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

> >   3. There were a number of alloc-and-copy instances. The copy part is
> >      the same as (2) above, but you have to repeat the size, which is
> >      potentially error-prone. I wonder if we would want something like:
> > 
> >        #define ALLOC_COPY(dst, src) do { \
> >          (dst) = xmalloc(sizeof(*(dst))); \
> > 	 COPY_ARRAY(dst, src, 1); \
> >        while(0)
> > 
> >      That avoids having to specify the size at all, and triggers a
> >      compile-time error if "src" and "dst" point to objects of different
> >      sizes.
> 
> Or you could call it DUP or similar.  And you could use ALLOC_ARRAY in
> its definition and let it infer the size implicitly (don't worry too
> much about the multiplication with one):
> 
> 	#define DUPLICATE_ARRAY(dst, src, n) do {	\
> 		ALLOC_ARRAY((dst), (n));		\
> 		COPY_ARRAY((dst), (src), (n));		\
> 	} while (0)
> 	#define DUPLICATE(dst, src) DUPLICATE_ARRAY((dst), (src), 1)
> 
> But do we even want such a thing?  Duplicating objects should be rare, and
> keeping allocation and assignment/copying separate makes for more flexible
> building blocks.  Adding ALLOC (and CALLOC) for single objects could be more
> widely useful, I think.

There's no reason you can't have both the building blocks and the thing
that is built for them. I think there are 5 spots that would use
DUPLICATE(), but I agree that there are more which could use ALLOC().

I'd be more worried that we're slowly drifting away from idiomatic C.
If it's safer, that's good. But if it makes it hard for people new to
the project to figure out what the hell is going on, that may be not so
good.

Here's the list of DUPLICATE spots, for reference.

---
diff --git a/builtin/blame.c b/builtin/blame.c
index f7aa95f4b..f0ac1c511 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -661,8 +661,8 @@ static struct origin *find_rename(struct scoreboard *sb,
 static void add_blame_entry(struct blame_entry ***queue,
 			    const struct blame_entry *src)
 {
-	struct blame_entry *e = xmalloc(sizeof(*e));
-	memcpy(e, src, sizeof(*e));
+	struct blame_entry *e;
+	DUPLICATE(e, src);
 	origin_incref(e->suspect);
 
 	e->next = **queue;
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 72c815844..d6cb893cf 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -206,8 +206,8 @@ static void llist_sorted_difference_inplace(struct llist *A,
 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));
+	struct pack_list *p;
+	DUPLICATE(p, entry);
 	p->next = *pl;
 	*pl = p;
 	return p;
@@ -238,8 +238,7 @@ static struct pack_list * pack_list_difference(const struct pack_list *A,
 			return pack_list_difference(A->next, B);
 		pl = pl->next;
 	}
-	ret = xmalloc(sizeof(struct pack_list));
-	memcpy(ret, A, sizeof(struct pack_list));
+	DUPLICATE(ret, A);
 	ret->next = pack_list_difference(A->next, B);
 	return ret;
 }
@@ -450,8 +449,7 @@ static void minimize(struct pack_list **min)
 		perm_all = perm = get_permutations(non_unique, n);
 		while (perm) {
 			if (is_superset(perm->pl, missing)) {
-				new_perm = xmalloc(sizeof(struct pll));
-				memcpy(new_perm, perm, sizeof(struct pll));
+				DUPLICATE(new_perm, perm);
 				new_perm->next = perm_ok;
 				perm_ok = new_perm;
 			}
diff --git a/diff.c b/diff.c
index 051761be4..dfe02f403 100644
--- a/diff.c
+++ b/diff.c
@@ -1168,8 +1168,8 @@ static void init_diff_words_data(struct emit_callback *ecbdata,
 				 struct diff_filespec *two)
 {
 	int i;
-	struct diff_options *o = xmalloc(sizeof(struct diff_options));
-	memcpy(o, orig_opts, sizeof(struct diff_options));
+	struct diff_options *o;
+	DUPLICATE(o, orig_opts);
 
 	ecbdata->diff_words =
 		xcalloc(1, sizeof(struct diff_words_data));