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);
}