{"thread":{"id":"44995","subject":"[PATCH 0/5] introduce SWAP macro","startedAt":"2017-01-28T21:31:01Z","lastAt":"2017-05-02T05:29:58Z","messageCount":50,"participants":["René Scharfe","Johannes Schindelin","Johannes Sixt","Brandon Williams","Junio C Hamano","Jeff King","Ramsay Jones","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"310493","messageId":"8ef4c833-45bd-6831-0683-6d01f30aa518@web.de","threadId":"44995","inReplyTo":null,"subject":"[PATCH 0/5] introduce SWAP macro","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-01-28T21:13:28Z","receivedAt":"2017-01-28T21:31:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Exchanging the value of two variables requires declaring a temporary\nvariable and repeating their names. The swap macro in apply.c\nsimplifies this for its callers without changing the compiled binary.\nPolish this macro and export it, then use it throughout the code to\nreduce repetition and hide the declaration of temporary variables.\n\n add SWAP macro\n apply: use SWAP macro\n use SWAP macro\n diff: use SWAP macro\n graph: use SWAP macro\n\n apply.c | 23 +++++++----------------\n builtin/diff-tree.c | 4 +---\n builtin/diff.c | 9 +++------\n contrib/coccinelle/swap.cocci | 28 ++++++++++++++++++++++++++++\n diff-no-index.c | 6 ++----\n diff.c | 12 ++++--------\n git-compat-util.h | 10 ++++++++++\n graph.c | 11 ++---------\n line-range.c | 3 +--\n merge-recursive.c | 5 +----\n object.c | 4 +---\n pack-revindex.c | 5 +----\n prio-queue.c | 4 +---\n strbuf.h | 4 +---\n 14 files changed, 63 insertions(+), 65 deletions(-)\n create mode 100644 contrib/coccinelle/swap.cocci\n\n-- \n2.11.0\n\n"},{"id":"310494","messageId":"0bdb58a6-3a7f-2218-4b70-c591ae90e95e@web.de","threadId":"44995","inReplyTo":"8ef4c833-45bd-6831-0683-6d01f30aa518@web.de","subject":"[PATCH 1/5] add SWAP macro","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-01-28T21:38:21Z","receivedAt":"2017-01-28T21:39:16Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a macro for exchanging the values of variables. It allows users\nto avoid repetition and takes care of the temporary variable for them.\nIt also makes sure that the storage sizes of its two parameters are the\nsame. Its memcpy(1) calls are optimized away by current compilers.\n\nAlso add a conservative semantic patch for transforming only swaps of\nvariables of the same type.\n\nSigned-off-by: Rene Scharfe \n---\n contrib/coccinelle/swap.cocci | 28 ++++++++++++++++++++++++++++\n git-compat-util.h | 10 ++++++++++\n 2 files changed, 38 insertions(+)\n create mode 100644 contrib/coccinelle/swap.cocci\n\ndiff --git a/contrib/coccinelle/swap.cocci b/contrib/coccinelle/swap.cocci\nnew file mode 100644\nindex 0000000000..a0934d1fda\n--- /dev/null\n+++ b/contrib/coccinelle/swap.cocci\n@@ -0,0 +1,28 @@\n+@ swap_with_declaration @\n+type T;\n+identifier tmp;\n+T a, b;\n+@@\n+- T tmp = a;\n++ T tmp;\n++ tmp = a;\n+ a = b;\n+ b = tmp;\n+\n+@ swap @\n+type T;\n+T tmp, a, b;\n+@@\n+- tmp = a;\n+- a = b;\n+- b = tmp;\n++ SWAP(a, b);\n+\n+@ extends swap @\n+identifier unused;\n+@@\n+ {\n+ ...\n+- T unused;\n+ ... when != unused\n+ }\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 87237b092b..66cd466eea 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -527,6 +527,16 @@ static inline int ends_with(const char *str, const char *suffix)\n \treturn strip_suffix(str, suffix, &len);\n }\n \n+#define SWAP(a, b) do {\t\t\t\t\t\t\\\n+\tvoid *_swap_a_ptr = &(a);\t\t\t\t\\\n+\tvoid *_swap_b_ptr = &(b);\t\t\t\t\\\n+\tunsigned char _swap_buffer[sizeof(a)];\t\t\t\\\n+\tmemcpy(_swap_buffer, _swap_a_ptr, sizeof(a));\t\t\\\n+\tmemcpy(_swap_a_ptr, _swap_b_ptr, sizeof(a) +\t\t\\\n+\t BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b)));\t\\\n+\tmemcpy(_swap_b_ptr, _swap_buffer, sizeof(a));\t\t\\\n+} while (0)\n+\n #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)\n \n #ifndef PROT_READ\n-- \n2.11.0\n\n"},{"id":"310495","messageId":"943fb53f-c602-7c04-ca9a-7d2b1bd68e77@web.de","threadId":"44995","inReplyTo":"8ef4c833-45bd-6831-0683-6d01f30aa518@web.de","subject":"[PATCH 2/5] apply: use SWAP macro","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-01-28T21:40:06Z","receivedAt":"2017-01-28T21:40:29Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use the exported macro SWAP instead of the file-scoped macro swap and\nremove the latter's definition.\n\nSigned-off-by: Rene Scharfe \n---\n apply.c | 23 +++++++----------------\n 1 file changed, 7 insertions(+), 16 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 2ed808d429..0e2caeab9c 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -2187,29 +2187,20 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si\n \treturn offset + hdrsize + patchsize;\n }\n \n-#define swap(a,b) myswap((a),(b),sizeof(a))\n-\n-#define myswap(a, b, size) do {\t\t\\\n-\tunsigned char mytmp[size];\t\\\n-\tmemcpy(mytmp, &a, size);\t\t\\\n-\tmemcpy(&a, &b, size);\t\t\\\n-\tmemcpy(&b, mytmp, size);\t\t\\\n-} while (0)\n-\n static void reverse_patches(struct patch *p)\n {\n \tfor (; p; p = p->next) {\n \t\tstruct fragment *frag = p->fragments;\n \n-\t\tswap(p->new_name, p->old_name);\n-\t\tswap(p->new_mode, p->old_mode);\n-\t\tswap(p->is_new, p->is_delete);\n-\t\tswap(p->lines_added, p->lines_deleted);\n-\t\tswap(p->old_sha1_prefix, p->new_sha1_prefix);\n+\t\tSWAP(p->new_name, p->old_name);\n+\t\tSWAP(p->new_mode, p->old_mode);\n+\t\tSWAP(p->is_new, p->is_delete);\n+\t\tSWAP(p->lines_added, p->lines_deleted);\n+\t\tSWAP(p->old_sha1_prefix, p->new_sha1_prefix);\n \n \t\tfor (; frag; frag = frag->next) {\n-\t\t\tswap(frag->newpos, frag->oldpos);\n-\t\t\tswap(frag->newlines, frag->oldlines);\n+\t\t\tSWAP(frag->newpos, frag->oldpos);\n+\t\t\tSWAP(frag->newlines, frag->oldlines);\n \t\t}\n \t}\n }\n-- \n2.11.0\n\n"},{"id":"310496","messageId":"187c2b39-40cf-7e07-b489-d40cdf5f9145@web.de","threadId":"44995","inReplyTo":"8ef4c833-45bd-6831-0683-6d01f30aa518@web.de","subject":"[PATCH 3/5] use SWAP macro","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-01-28T21:40:58Z","receivedAt":"2017-01-28T21:42:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Apply the semantic patch swap.cocci to convert hand-rolled swaps to use\nthe macro SWAP. The resulting code is shorter and easier to read, the\nobject code is effectively unchanged.\n\nThe patch for object.c had to be hand-edited in order to preserve the\ncomment before the change; Coccinelle tried to eat it for some reason.\n\nSigned-off-by: Rene Scharfe \n---\n builtin/diff-tree.c | 4 +---\n builtin/diff.c | 9 +++------\n diff-no-index.c | 3 +--\n diff.c | 8 +++-----\n graph.c | 5 +----\n line-range.c | 3 +--\n merge-recursive.c | 5 +----\n object.c | 4 +---\n pack-revindex.c | 5 +----\n prio-queue.c | 4 +---\n strbuf.h | 4 +---\n 11 files changed, 15 insertions(+), 39 deletions(-)\n\ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex 806dd7a885..8ce00480cd 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -147,9 +147,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \t\ttree1 = opt->pending.objects[0].item;\n \t\ttree2 = opt->pending.objects[1].item;\n \t\tif (tree2->flags & UNINTERESTING) {\n-\t\t\tstruct object *tmp = tree2;\n-\t\t\ttree2 = tree1;\n-\t\t\ttree1 = tmp;\n+\t\t\tSWAP(tree2, tree1);\n \t\t}\n \t\tdiff_tree_sha1(tree1->oid.hash,\n \t\t\t tree2->oid.hash,\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 7f91f6d226..3d64b85337 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -45,12 +45,9 @@ static void stuff_change(struct diff_options *opt,\n \t\treturn;\n \n \tif (DIFF_OPT_TST(opt, REVERSE_DIFF)) {\n-\t\tunsigned tmp;\n-\t\tconst unsigned char *tmp_u;\n-\t\tconst char *tmp_c;\n-\t\ttmp = old_mode; old_mode = new_mode; new_mode = tmp;\n-\t\ttmp_u = old_sha1; old_sha1 = new_sha1; new_sha1 = tmp_u;\n-\t\ttmp_c = old_name; old_name = new_name; new_name = tmp_c;\n+\t\tSWAP(old_mode, new_mode);\n+\t\tSWAP(old_sha1, new_sha1);\n+\t\tSWAP(old_name, new_name);\n \t}\n \n \tif (opt->prefix &&\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex f420786039..1ae09894d7 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -186,9 +186,8 @@ static int queue_diff(struct diff_options *o,\n \n \t\tif (DIFF_OPT_TST(o, REVERSE_DIFF)) {\n \t\t\tunsigned tmp;\n-\t\t\tconst char *tmp_c;\n \t\t\ttmp = mode1; mode1 = mode2; mode2 = tmp;\n-\t\t\ttmp_c = name1; name1 = name2; name2 = tmp_c;\n+\t\t\tSWAP(name1, name2);\n \t\t}\n \n \t\td1 = noindex_filespec(name1, mode1);\ndiff --git a/diff.c b/diff.c\nindex f08cd8e033..9de1ba264f 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5118,13 +5118,11 @@ void diff_change(struct diff_options *options,\n \n \tif (DIFF_OPT_TST(options, REVERSE_DIFF)) {\n \t\tunsigned tmp;\n-\t\tconst unsigned char *tmp_c;\n-\t\ttmp = old_mode; old_mode = new_mode; new_mode = tmp;\n-\t\ttmp_c = old_sha1; old_sha1 = new_sha1; new_sha1 = tmp_c;\n+\t\tSWAP(old_mode, new_mode);\n+\t\tSWAP(old_sha1, new_sha1);\n \t\ttmp = old_sha1_valid; old_sha1_valid = new_sha1_valid;\n \t\t\tnew_sha1_valid = tmp;\n-\t\ttmp = old_dirty_submodule; old_dirty_submodule = new_dirty_submodule;\n-\t\t\tnew_dirty_submodule = tmp;\n+\t\tSWAP(old_dirty_submodule, new_dirty_submodule);\n \t}\n \n \tif (options->prefix &&\ndiff --git a/graph.c b/graph.c\nindex d4e8519c90..4c722303d2 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -997,7 +997,6 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct strbuf\n static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf *sb)\n {\n \tint i;\n-\tint *tmp_mapping;\n \tshort used_horizontal = 0;\n \tint horizontal_edge = -1;\n \tint horizontal_edge_target = -1;\n@@ -1132,9 +1131,7 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf\n \t/*\n \t * Swap mapping and new_mapping\n \t */\n-\ttmp_mapping = graph->mapping;\n-\tgraph->mapping = graph->new_mapping;\n-\tgraph->new_mapping = tmp_mapping;\n+\tSWAP(graph->mapping, graph->new_mapping);\n \n \t/*\n \t * If graph->mapping indicates that all of the branch lines\ndiff --git a/line-range.c b/line-range.c\nindex de4e32f942..323399d16c 100644\n--- a/line-range.c\n+++ b/line-range.c\n@@ -269,8 +269,7 @@ int parse_range_arg(const char *arg, nth_line_fn_t nth_line_cb,\n \t\treturn -1;\n \n \tif (*begin && *end && *end < *begin) {\n-\t\tlong tmp;\n-\t\ttmp = *end; *end = *begin; *begin = tmp;\n+\t\tSWAP(*end, *begin);\n \t}\n \n \treturn 0;\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex d327209443..b7ff1ada3c 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1390,14 +1390,11 @@ static int process_renames(struct merge_options *o,\n \t\t\tbranch1 = o->branch1;\n \t\t\tbranch2 = o->branch2;\n \t\t} else {\n-\t\t\tstruct rename *tmp;\n \t\t\trenames1 = b_renames;\n \t\t\trenames2Dst = &a_by_dst;\n \t\t\tbranch1 = o->branch2;\n \t\t\tbranch2 = o->branch1;\n-\t\t\ttmp = ren2;\n-\t\t\tren2 = ren1;\n-\t\t\tren1 = tmp;\n+\t\t\tSWAP(ren2, ren1);\n \t\t}\n \n \t\tif (ren1->processed)\ndiff --git a/object.c b/object.c\nindex 67d9a9e221..e680d881a4 100644\n--- a/object.c\n+++ b/object.c\n@@ -104,9 +104,7 @@ struct object *lookup_object(const unsigned char *sha1)\n \t\t * that we do not need to walk the hash table the next\n \t\t * time we look for it.\n \t\t */\n-\t\tstruct object *tmp = obj_hash[i];\n-\t\tobj_hash[i] = obj_hash[first];\n-\t\tobj_hash[first] = tmp;\n+\t\tSWAP(obj_hash[i], obj_hash[first]);\n \t}\n \treturn obj;\n }\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex 6bc7c94033..1b7ebd8d7e 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -59,7 +59,6 @@ static void sort_revindex(struct revindex_entry *entries, unsigned n, off_t max)\n \t * be a no-op, as everybody lands in the same zero-th bucket.\n \t */\n \tfor (bits = 0; max >> bits; bits += DIGIT_SIZE) {\n-\t\tstruct revindex_entry *swap;\n \t\tunsigned i;\n \n \t\tmemset(pos, 0, BUCKETS * sizeof(*pos));\n@@ -97,9 +96,7 @@ static void sort_revindex(struct revindex_entry *entries, unsigned n, off_t max)\n \t\t * Now \"to\" contains the most sorted list, so we swap \"from\" and\n \t\t * \"to\" for the next iteration.\n \t\t */\n-\t\tswap = from;\n-\t\tfrom = to;\n-\t\tto = swap;\n+\t\tSWAP(from, to);\n \t}\n \n \t/*\ndiff --git a/prio-queue.c b/prio-queue.c\nindex e4365b00d6..17252d231b 100644\n--- a/prio-queue.c\n+++ b/prio-queue.c\n@@ -12,9 +12,7 @@ static inline int compare(struct prio_queue *queue, int i, int j)\n \n static inline void swap(struct prio_queue *queue, int i, int j)\n {\n-\tstruct prio_queue_entry tmp = queue->array[i];\n-\tqueue->array[i] = queue->array[j];\n-\tqueue->array[j] = tmp;\n+\tSWAP(queue->array[i], queue->array[j]);\n }\n \n void prio_queue_reverse(struct prio_queue *queue)\ndiff --git a/strbuf.h b/strbuf.h\nindex 2262b12683..cf1b5409e7 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -109,9 +109,7 @@ extern void strbuf_attach(struct strbuf *, void *, size_t, size_t);\n */\n static inline void strbuf_swap(struct strbuf *a, struct strbuf *b)\n {\n-\tstruct strbuf tmp = *a;\n-\t*a = *b;\n-\t*b = tmp;\n+\tSWAP(*a, *b);\n }\n \n \n-- \n2.11.0\n\n"},{"id":"310497","messageId":"84944ecd-d14e-b5e9-7566-9ab2b68c02fb@web.de","threadId":"44995","inReplyTo":"8ef4c833-45bd-6831-0683-6d01f30aa518@web.de","subject":"[PATCH 4/5] diff: use SWAP macro","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-01-28T21:41:47Z","receivedAt":"2017-01-28T21:42:27Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use the macro SWAP to exchange the value of pairs of variables instead\nof swapping them manually with the help of a temporary variable. The\nresulting code is shorter and easier to read.\n\nThe two cases were not transformed by the semantic patch swap.cocci\nbecause it's extra careful and handles only cases where the types of all\nvariables are the same -- and here we swap two ints and use an unsigned\ntemporary variable for that. Nevertheless the conversion is safe, as\nthe value range is preserved with and without the patch.\n\nSigned-off-by: Rene Scharfe \n---\n diff-no-index.c | 3 +--\n diff.c | 4 +---\n 2 files changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 1ae09894d7..df762fd0f7 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -185,8 +185,7 @@ static int queue_diff(struct diff_options *o,\n \t\tstruct diff_filespec *d1, *d2;\n \n \t\tif (DIFF_OPT_TST(o, REVERSE_DIFF)) {\n-\t\t\tunsigned tmp;\n-\t\t\ttmp = mode1; mode1 = mode2; mode2 = tmp;\n+\t\t\tSWAP(mode1, mode2);\n \t\t\tSWAP(name1, name2);\n \t\t}\n \ndiff --git a/diff.c b/diff.c\nindex 9de1ba264f..6c4f3f6b72 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5117,11 +5117,9 @@ void diff_change(struct diff_options *options,\n \t\treturn;\n \n \tif (DIFF_OPT_TST(options, REVERSE_DIFF)) {\n-\t\tunsigned tmp;\n \t\tSWAP(old_mode, new_mode);\n \t\tSWAP(old_sha1, new_sha1);\n-\t\ttmp = old_sha1_valid; old_sha1_valid = new_sha1_valid;\n-\t\t\tnew_sha1_valid = tmp;\n+\t\tSWAP(old_sha1_valid, new_sha1_valid);\n \t\tSWAP(old_dirty_submodule, new_dirty_submodule);\n \t}\n \n-- \n2.11.0\n\n"},{"id":"310498","messageId":"af5a7205-7703-f5ad-4ea2-b20ab4c01c80@web.de","threadId":"44995","inReplyTo":"8ef4c833-45bd-6831-0683-6d01f30aa518@web.de","subject":"[PATCH 5/5] graph: use SWAP macro","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-01-28T21:42:15Z","receivedAt":"2017-01-28T21:43:04Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Exchange the values of graph->columns and graph->new_columns using the\nmacro SWAP instead of hand-rolled code. The result is shorter and\neasier to read.\n\nThis transformation was not done by the semantic patch swap.cocci\nbecause there's an unrelated statement between the second and the last\nstep of the exchange, so it didn't match the expected pattern.\n\nSigned-off-by: Rene Scharfe \n---\n graph.c | 6 +-----\n 1 file changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/graph.c b/graph.c\nindex 4c722303d2..29b0f51dc5 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -463,7 +463,6 @@ static void graph_update_width(struct git_graph *graph,\n static void graph_update_columns(struct git_graph *graph)\n {\n \tstruct commit_list *parent;\n-\tstruct column *tmp_columns;\n \tint max_new_columns;\n \tint mapping_idx;\n \tint i, seen_this, is_commit_in_columns;\n@@ -476,11 +475,8 @@ static void graph_update_columns(struct git_graph *graph)\n \t * We'll re-use the old columns array as storage to compute the new\n \t * columns list for the commit after this one.\n \t */\n-\ttmp_columns = graph->columns;\n-\tgraph->columns = graph->new_columns;\n+\tSWAP(graph->columns, graph->new_columns);\n \tgraph->num_columns = graph->num_new_columns;\n-\n-\tgraph->new_columns = tmp_columns;\n \tgraph->num_new_columns = 0;\n \n \t/*\n-- \n2.11.0\n\n"},{"id":"310526","messageId":"alpine.DEB.2.20.1701301637570.3469@virtualbox","threadId":"44995","inReplyTo":"0bdb58a6-3a7f-2218-4b70-c591ae90e95e@web.de","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-30T15:39:38Z","receivedAt":"2017-01-30T15:41:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Sat, 28 Jan 2017, René Scharfe wrote:\n\n> Add a macro for exchanging the values of variables. It allows users to\n> avoid repetition and takes care of the temporary variable for them. It\n> also makes sure that the storage sizes of its two parameters are the\n> same. Its memcpy(1) calls are optimized away by current compilers.\n\nHow certain are we that \"current compilers\" optimize that away? And about\nwhich \"current compilers\" are we talking? GCC? GCC 6? Clang? Clang 3?\nClang 3.8.*? Visual C 2005+?\n\nCiao,\nDscho"},{"id":"310527","messageId":"alpine.DEB.2.20.1701301643260.3469@virtualbox","threadId":"44995","inReplyTo":"0bdb58a6-3a7f-2218-4b70-c591ae90e95e@web.de","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-30T16:01:58Z","receivedAt":"2017-01-30T16:03:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Sat, 28 Jan 2017, René Scharfe wrote:\n\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 87237b092b..66cd466eea 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -527,6 +527,16 @@ static inline int ends_with(const char *str, const char *suffix)\n> \treturn strip_suffix(str, suffix, &len);\n> }\n> \n> +#define SWAP(a, b) do {\t\t\t\t\t\t\\\n> +\tvoid *_swap_a_ptr = &(a);\t\t\t\t\\\n> +\tvoid *_swap_b_ptr = &(b);\t\t\t\t\\\n> +\tunsigned char _swap_buffer[sizeof(a)];\t\t\t\\\n> +\tmemcpy(_swap_buffer, _swap_a_ptr, sizeof(a));\t\t\\\n> +\tmemcpy(_swap_a_ptr, _swap_b_ptr, sizeof(a) +\t\t\\\n> +\t BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b)));\t\\\n> +\tmemcpy(_swap_b_ptr, _swap_buffer, sizeof(a));\t\t\\\n> +} while (0)\n> +\n> #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)\n\nIt may seem as a matter of taste, or maybe not: I prefer this without the\n_swap_a_ptr (and I would also prefer not to use identifiers starting with\nan underscore, as section 7.1.3 Reserved Identifiers of the C99 standard\nsays they are reserved):\n\n+#define SWAP(a, b) do {\t\t\t\t\t\t\\\n+\tunsigned char swap_buffer_[sizeof(a)];\t\t\t\\\n+\tmemcpy(swap_buffer_, &(a), sizeof(a));\t\t\t\\\n+\tmemcpy(&(a), &(b), sizeof(a) +\t\t\t\t\\\n+\t BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b)));\t\\\n+\tmemcpy(&(b), swap_buffer_, sizeof(a));\t\t\t\\\n+} while (0)\n\nOne idea to address the concern that not all C compilers people use to\nbuild Git may optimize away those memcpy()s: we could also introduce a\nSWAP_PRIMITIVE_TYPE (or SWAP2 or SIMPLE_SWAP or whatever) that accepts\nonly primitive types. But since __typeof__() is not portable...\n\nCiao,\nDscho"},{"id":"310528","messageId":"alpine.DEB.2.20.1701301702120.3469@virtualbox","threadId":"44995","inReplyTo":"187c2b39-40cf-7e07-b489-d40cdf5f9145@web.de","subject":"Re: [PATCH 3/5] use SWAP macro","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-30T16:03:15Z","receivedAt":"2017-01-30T16:04:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Sat, 28 Jan 2017, René Scharfe wrote:\n\n> diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\n> index 806dd7a885..8ce00480cd 100644\n> --- a/builtin/diff-tree.c\n> +++ b/builtin/diff-tree.c\n> @@ -147,9 +147,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n> \t\ttree1 = opt->pending.objects[0].item;\n> \t\ttree2 = opt->pending.objects[1].item;\n> \t\tif (tree2->flags & UNINTERESTING) {\n> -\t\t\tstruct object *tmp = tree2;\n> -\t\t\ttree2 = tree1;\n> -\t\t\ttree1 = tmp;\n> +\t\t\tSWAP(tree2, tree1);\n> \t\t}\n\nIs there a way to transform away the curly braces for blocks that become\nsingle-line blocks, too?\n\nCiao,\nDscho"},{"id":"310529","messageId":"alpine.DEB.2.20.1701301704110.3469@virtualbox","threadId":"44995","inReplyTo":"84944ecd-d14e-b5e9-7566-9ab2b68c02fb@web.de","subject":"Re: [PATCH 4/5] diff: use SWAP macro","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-30T16:04:56Z","receivedAt":"2017-01-30T16:12:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Sat, 28 Jan 2017, René Scharfe wrote:\n\n> Use the macro SWAP to exchange the value of pairs of variables instead\n> of swapping them manually with the help of a temporary variable. The\n> resulting code is shorter and easier to read.\n> \n> The two cases were not transformed by the semantic patch swap.cocci\n> because it's extra careful and handles only cases where the types of all\n> variables are the same -- and here we swap two ints and use an unsigned\n> temporary variable for that. Nevertheless the conversion is safe, as\n> the value range is preserved with and without the patch.\n\nOne way to make this more obvious would be to change the type to signed\nfirst, and then transform (which then would catch these cases too,\nright?).\n\nCiao,\nDscho"},{"id":"310531","messageId":"alpine.DEB.2.20.1701301714450.3469@virtualbox","threadId":"44995","inReplyTo":"af5a7205-7703-f5ad-4ea2-b20ab4c01c80@web.de","subject":"Re: [PATCH 5/5] graph: use SWAP macro","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-30T16:16:14Z","receivedAt":"2017-01-30T16:23:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Sat, 28 Jan 2017, René Scharfe wrote:\n\n> Exchange the values of graph->columns and graph->new_columns using the\n> macro SWAP instead of hand-rolled code. The result is shorter and\n> easier to read.\n> \n> This transformation was not done by the semantic patch swap.cocci\n> because there's an unrelated statement between the second and the last\n> step of the exchange, so it didn't match the expected pattern.\n\nIs it really true that Coccinelle cannot be told to look for a code block\nthat declares a variable that is then used *only* in the lines we want to\nmatch and replace?\n\nI never used the tool, and a quick web search did not clarify the picture,\neither...\n\nCiao,\nDscho"},{"id":"310532","messageId":"9bcae427-d654-a671-4368-030150168102@web.de","threadId":"44995","inReplyTo":"alpine.DEB.2.20.1701301637570.3469@virtualbox","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-01-30T16:48:00Z","receivedAt":"2017-01-30T16:51:17Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.01.2017 um 16:39 schrieb Johannes Schindelin:\n> Hi René,\n>\n> On Sat, 28 Jan 2017, René Scharfe wrote:\n>\n>> Add a macro for exchanging the values of variables. It allows users to\n>> avoid repetition and takes care of the temporary variable for them. It\n>> also makes sure that the storage sizes of its two parameters are the\n>> same. Its memcpy(1) calls are optimized away by current compilers.\n>\n> How certain are we that \"current compilers\" optimize that away? And about\n> which \"current compilers\" are we talking? GCC? GCC 6? Clang? Clang 3?\n> Clang 3.8.*? Visual C 2005+?\n\nGCC 4.4.7 and clang 3.2 on x86-64 compile the macro to the same object \ncode as a manual swap ; see https://godbolt.org/g/F4b9M9. Both were \nreleased 2012. That website doesn't offer Visual C(++), but since you \nadded the original macro in e5a94313c0 (\"Teach git-apply about '-R'\", \n2006) I guess we have that part covered. ;)\n\nRené\n"},{"id":"310535","messageId":"11e27272-c1cd-a127-eb44-385d827af63d@web.de","threadId":"44995","inReplyTo":"alpine.DEB.2.20.1701301643260.3469@virtualbox","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-01-30T16:59:37Z","receivedAt":"2017-01-30T17:00:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.01.2017 um 17:01 schrieb Johannes Schindelin:\n> Hi René,\n>\n> On Sat, 28 Jan 2017, René Scharfe wrote:\n>\n>> diff --git a/git-compat-util.h b/git-compat-util.h\n>> index 87237b092b..66cd466eea 100644\n>> --- a/git-compat-util.h\n>> +++ b/git-compat-util.h\n>> @@ -527,6 +527,16 @@ static inline int ends_with(const char *str, const char *suffix)\n>> \treturn strip_suffix(str, suffix, &len);\n>> }\n>>\n>> +#define SWAP(a, b) do {\t\t\t\t\t\t\\\n>> +\tvoid *_swap_a_ptr = &(a);\t\t\t\t\\\n>> +\tvoid *_swap_b_ptr = &(b);\t\t\t\t\\\n>> +\tunsigned char _swap_buffer[sizeof(a)];\t\t\t\\\n>> +\tmemcpy(_swap_buffer, _swap_a_ptr, sizeof(a));\t\t\\\n>> +\tmemcpy(_swap_a_ptr, _swap_b_ptr, sizeof(a) +\t\t\\\n>> +\t BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b)));\t\\\n>> +\tmemcpy(_swap_b_ptr, _swap_buffer, sizeof(a));\t\t\\\n>> +} while (0)\n>> +\n>> #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)\n>\n> It may seem as a matter of taste, or maybe not: I prefer this without the\n> _swap_a_ptr (and I would also prefer not to use identifiers starting with\n> an underscore, as section 7.1.3 Reserved Identifiers of the C99 standard\n> says they are reserved):\n>\n> +#define SWAP(a, b) do {\t\t\t\t\t\t\\\n> +\tunsigned char swap_buffer_[sizeof(a)];\t\t\t\\\n> +\tmemcpy(swap_buffer_, &(a), sizeof(a));\t\t\t\\\n> +\tmemcpy(&(a), &(b), sizeof(a) +\t\t\t\t\\\n> +\t BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b)));\t\\\n> +\tmemcpy(&(b), swap_buffer_, sizeof(a));\t\t\t\\\n> +} while (0)\n\nWe can move the underscore to the end, but using a and b directly will \ngive surprising results if the parameters have side effects. E.g. if \nyou want to swap the first two elements of two arrays you might want to \ndo this:\n\n\tSWAP(*x++, *y++);\n\tSWAP(*x++, *y++);\n\nAnd that would increment twice as much as one would guess and access \nunexpected elements.\n\n> One idea to address the concern that not all C compilers people use to\n> build Git may optimize away those memcpy()s: we could also introduce a\n> SWAP_PRIMITIVE_TYPE (or SWAP2 or SIMPLE_SWAP or whatever) that accepts\n> only primitive types. But since __typeof__() is not portable...\n\nI wouldn't worry too much about such a solution before seeing that SWAP \n(even with memcpy(3) -- this function is probably optimized quite \nheavily on most platforms) causes an actual performance problem.\n\nRené\n"},{"id":"310538","messageId":"3058e7e1-c779-0183-9394-e9cd5e05cc3c@web.de","threadId":"44995","inReplyTo":"alpine.DEB.2.20.1701301704110.3469@virtualbox","subject":"Re: [PATCH 4/5] diff: use SWAP macro","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-01-30T17:26:20Z","receivedAt":"2017-01-30T17:27:00Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.01.2017 um 17:04 schrieb Johannes Schindelin:\n> Hi René,\n>\n> On Sat, 28 Jan 2017, René Scharfe wrote:\n>\n>> Use the macro SWAP to exchange the value of pairs of variables instead\n>> of swapping them manually with the help of a temporary variable. The\n>> resulting code is shorter and easier to read.\n>>\n>> The two cases were not transformed by the semantic patch swap.cocci\n>> because it's extra careful and handles only cases where the types of all\n>> variables are the same -- and here we swap two ints and use an unsigned\n>> temporary variable for that. Nevertheless the conversion is safe, as\n>> the value range is preserved with and without the patch.\n>\n> One way to make this more obvious would be to change the type to signed\n> first, and then transform (which then would catch these cases too,\n> right?).\n\nI'm not sure it would be more obvious, but it would certainly make the \ntype change more explicit. In diff-index.c we might even want to change \nthe type of the swapped values from int to unsigned, which is more \nfitting for file modes. In diff.c we'd need to add a separate variable, \nas tmp is shared with other (unsigned) swaps.\n\nRené\n\n"},{"id":"310540","messageId":"65b00711-bea1-a4e6-290b-1975319911c4@web.de","threadId":"44995","inReplyTo":"alpine.DEB.2.20.1701301702120.3469@virtualbox","subject":"Re: [PATCH 3/5] use SWAP macro","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-01-30T17:18:21Z","receivedAt":"2017-01-30T17:36:37Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.01.2017 um 17:03 schrieb Johannes Schindelin:\n> Hi René,\n>\n> On Sat, 28 Jan 2017, René Scharfe wrote:\n>\n>> diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\n>> index 806dd7a885..8ce00480cd 100644\n>> --- a/builtin/diff-tree.c\n>> +++ b/builtin/diff-tree.c\n>> @@ -147,9 +147,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n>> \t\ttree1 = opt->pending.objects[0].item;\n>> \t\ttree2 = opt->pending.objects[1].item;\n>> \t\tif (tree2->flags & UNINTERESTING) {\n>> -\t\t\tstruct object *tmp = tree2;\n>> -\t\t\ttree2 = tree1;\n>> -\t\t\ttree1 = tmp;\n>> +\t\t\tSWAP(tree2, tree1);\n>> \t\t}\n>\n> Is there a way to transform away the curly braces for blocks that become\n> single-line blocks, too?\n\nInteresting question. I guess this can be done by using a Python script \n(see contrib/coccinelle/strbuf.cocci for an example). I'll leave this \nas homework for readers interested in Coccinelle, at least for a while. :)\n\nRené\n"},{"id":"310541","messageId":"63f1dcc0-729c-955b-63fa-859e441406bd@web.de","threadId":"44995","inReplyTo":"alpine.DEB.2.20.1701301714450.3469@virtualbox","subject":"Re: [PATCH 5/5] graph: use SWAP macro","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-01-30T17:41:01Z","receivedAt":"2017-01-30T17:48:14Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.01.2017 um 17:16 schrieb Johannes Schindelin:\n> Hi René,\n>\n> On Sat, 28 Jan 2017, René Scharfe wrote:\n>\n>> Exchange the values of graph->columns and graph->new_columns using the\n>> macro SWAP instead of hand-rolled code. The result is shorter and\n>> easier to read.\n>>\n>> This transformation was not done by the semantic patch swap.cocci\n>> because there's an unrelated statement between the second and the last\n>> step of the exchange, so it didn't match the expected pattern.\n>\n> Is it really true that Coccinelle cannot be told to look for a code block\n> that declares a variable that is then used *only* in the lines we want to\n> match and replace?\n\nHope I parsed your question correctly; my answer would be that it can't \nbe true because that's basically what the proposed semantic patch does:\n\n\t@ swap @\n\ttype T;\n\tT tmp, a, b;\n\t@@\n\t- tmp = a;\n\t- a = b;\n\t- b = tmp;\n\t+ SWAP(a, b);\n\n\t@ extends swap @\n\tidentifier unused;\n\t@@\n\t {\n\t ...\n\t- T unused;\n\t ... when != unused\n\t }\n\nThe first part (up to the \"+\") looks for a opportunities to use SWAP, \nand the second part looks for blocks where that transformation was done \nand we declare identifiers that are/became unused.\n\nIt did not match the code in graph.c because the pattern was basically:\n\n\ttmp = a;\n\ta = b;\n\tsomething = totally_different;\n\tb = tmp;\n\nCoccinelle can be told to ignore such unrelated code by adding \"... when \n!= tmp\" etc. (which matches context lines that don't reference tmp), but \nthat's slooow. (Perhaps I just did it wrong, though.)\n\nRené\n"},{"id":"310546","messageId":"aa653d57-4a97-ac50-b20c-f94ed43a22fb@kdbg.org","threadId":"44995","inReplyTo":"alpine.DEB.2.20.1701301643260.3469@virtualbox","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-01-30T18:41:37Z","receivedAt":"2017-01-30T18:41:57Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 30.01.2017 um 17:01 schrieb Johannes Schindelin:\n> On Sat, 28 Jan 2017, René Scharfe wrote:\n>> diff --git a/git-compat-util.h b/git-compat-util.h\n>> index 87237b092b..66cd466eea 100644\n>> --- a/git-compat-util.h\n>> +++ b/git-compat-util.h\n>> @@ -527,6 +527,16 @@ static inline int ends_with(const char *str, const char *suffix)\n>> \treturn strip_suffix(str, suffix, &len);\n>> }\n>>\n>> +#define SWAP(a, b) do {\t\t\t\t\t\t\\\n>> +\tvoid *_swap_a_ptr = &(a);\t\t\t\t\\\n>> +\tvoid *_swap_b_ptr = &(b);\t\t\t\t\\\n>> +\tunsigned char _swap_buffer[sizeof(a)];\t\t\t\\\n>> +\tmemcpy(_swap_buffer, _swap_a_ptr, sizeof(a));\t\t\\\n>> +\tmemcpy(_swap_a_ptr, _swap_b_ptr, sizeof(a) +\t\t\\\n>> +\t BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b)));\t\\\n>> +\tmemcpy(_swap_b_ptr, _swap_buffer, sizeof(a));\t\t\\\n>> +} while (0)\n>> +\n>> #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)\n>\n> It may seem as a matter of taste, or maybe not: I prefer this without the\n> _swap_a_ptr\n\nThe purpose of these pointers is certainly to avoid that the macro \narguments are evaluated more than once.\n\n-- Hannes\n\n"},{"id":"310553","messageId":"alpine.DEB.2.20.1701301806591.3469@virtualbox","threadId":"44995","inReplyTo":"9bcae427-d654-a671-4368-030150168102@web.de","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-30T20:48:29Z","receivedAt":"2017-01-30T20:49:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Mon, 30 Jan 2017, René Scharfe wrote:\n\n> Am 30.01.2017 um 16:39 schrieb Johannes Schindelin:\n>\n> > On Sat, 28 Jan 2017, René Scharfe wrote:\n> >\n> > > Add a macro for exchanging the values of variables. It allows users\n> > > to avoid repetition and takes care of the temporary variable for\n> > > them. It also makes sure that the storage sizes of its two\n> > > parameters are the same. Its memcpy(1) calls are optimized away by\n> > > current compilers.\n> >\n> > How certain are we that \"current compilers\" optimize that away? And\n> > about which \"current compilers\" are we talking? GCC? GCC 6? Clang?\n> > Clang 3? Clang 3.8.*? Visual C 2005+?\n> \n> GCC 4.4.7 and clang 3.2 on x86-64 compile the macro to the same object\n> code as a manual swap ; see https://godbolt.org/g/F4b9M9. Both were\n> released 2012.\n\nGood. Thank you.\n\n> That website doesn't offer Visual C(++), but since you added the\n> original macro in e5a94313c0 (\"Teach git-apply about '-R'\", 2006) I\n> guess we have that part covered. ;)\n\nBack then, I was not able to build Git using Visual C *at all*. It\nrequired a Herculean effort to make that happen, and it did not happen\nbefore the Git for Windows project was started in 2007.\n\nSo I tried to verify that Visual C optimizes this well, and oh my deity,\nthis was not easy. In Debug mode, it does not optimize, i.e. the memcpy()\nwill be called, even for simple 32-bit integers. In Release mode, Visual\nStudio's defaults turn on \"whole-program optimization\" which means that\nthe only swapping that is going on in the end is that the meaning of two\nregisters is swapped, i.e. the SWAP() macro is expanded to... no assembler\ncode at all.\n\nAfter trying this and that and something else, I finally ended up with the\nfile-scope optimized SWAP() resulting in this assembly code:\n\n00007FF791311000 mov eax,dword ptr [rcx] \n00007FF791311002 mov r8d,dword ptr [rdx] \n00007FF791311005 mov dword ptr [rcx],r8d \n00007FF791311008 mov dword ptr [rdx],eax \n\nHowever, recent events (including some discussions on this mailing list\nwhich had unfortunate consequences) made me trust my instincts more. And\nmy instincts tell me that it would be unwise to replace code that swaps\nprimitive types in the straight-forward way by code that relies on\nadvanced compiler optimization to generate efficient code, otherwise\nproducing very suboptimal code.\n\nThe commit you quoted embarrasses me, and I have no excuse for it. I would\nlove to see that myswap() ugliness fixed by replacing it with a construct\nthat is simpler, and generates good code even without any smart compiler.\n\nCiao,\nDscho"},{"id":"310556","messageId":"alpine.DEB.2.20.1701302158110.3469@virtualbox","threadId":"44995","inReplyTo":"aa653d57-4a97-ac50-b20c-f94ed43a22fb@kdbg.org","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-30T21:03:19Z","receivedAt":"2017-01-30T21:10:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Mon, 30 Jan 2017, Johannes Sixt wrote:\n\n> Am 30.01.2017 um 17:01 schrieb Johannes\n> Schindelin:\n> > On Sat, 28 Jan 2017, René Scharfe wrote:\n> > > diff --git a/git-compat-util.h\n> > > b/git-compat-util.h\n> > > index 87237b092b..66cd466eea 100644\n> > > --- a/git-compat-util.h\n> > > +++ b/git-compat-util.h\n> > > @@ -527,6 +527,16 @@ static inline int\n> > > @@ ends_with(const char *str, const char\n> > > @@ *suffix)\n> > > \treturn strip_suffix(str, suffix, &len);\n> > > }\n> > >\n> > > +#define SWAP(a, b) do {\n> > > \\\n> > > +\tvoid *_swap_a_ptr = &(a);\n> > > \\\n> > > +\tvoid *_swap_b_ptr = &(b);\n> > > \\\n> > > +\tunsigned char _swap_buffer[sizeof(a)];\n> > > \\\n> > > +\tmemcpy(_swap_buffer, _swap_a_ptr,\n> > > sizeof(a));\t\t\\\n> > > +\tmemcpy(_swap_a_ptr, _swap_b_ptr,\n> > > sizeof(a) +\t\t\\\n> > > +\t BUILD_ASSERT_OR_ZERO(sizeof(a)\n> > > == sizeof(b)));\t\\\n> > > +\tmemcpy(_swap_b_ptr, _swap_buffer,\n> > > sizeof(a));\t\t\\\n> > > +} while (0)\n> > > +\n> > > #if defined(NO_MMAP) ||\n> > > defined(USE_WIN32_MMAP)\n> >\n> > It may seem as a matter of taste, or maybe\n> > not: I prefer this without the\n> > _swap_a_ptr\n> \n> The purpose of these pointers is certainly to\n> avoid that the macro arguments are evaluated\n> more than once.\n\nI mistook \"a\" being used in sizeof(a) for breaking that assumption, but of\ncourse a is *not* evaluated in that case. It is curious, though, that an\nexpression like \"sizeof(a++)\" would not be rejected.\n\nFurther, what would SWAP(a++, b) do? Swap a and b, and *then* increment a?\n\nCiao,\nJohannes"},{"id":"310563","messageId":"6760493c-3684-b8bb-2c01-6621b8417246@web.de","threadId":"44995","inReplyTo":"alpine.DEB.2.20.1701301806591.3469@virtualbox","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-01-30T21:46:19Z","receivedAt":"2017-01-30T21:46:53Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.01.2017 um 21:48 schrieb Johannes Schindelin:\n> So I tried to verify that Visual C optimizes this well, and oh my deity,\n> this was not easy. In Debug mode, it does not optimize, i.e. the memcpy()\n> will be called, even for simple 32-bit integers. In Release mode, Visual\n> Studio's defaults turn on \"whole-program optimization\" which means that\n> the only swapping that is going on in the end is that the meaning of two\n> registers is swapped, i.e. the SWAP() macro is expanded to... no assembler\n> code at all.\n>\n> After trying this and that and something else, I finally ended up with the\n> file-scope optimized SWAP() resulting in this assembly code:\n>\n> 00007FF791311000 mov eax,dword ptr [rcx]\n> 00007FF791311002 mov r8d,dword ptr [rdx]\n> 00007FF791311005 mov dword ptr [rcx],r8d\n> 00007FF791311008 mov dword ptr [rdx],eax\n\nThis looks good.\n\n> However, recent events (including some discussions on this mailing list\n> which had unfortunate consequences) made me trust my instincts more. And\n> my instincts tell me that it would be unwise to replace code that swaps\n> primitive types in the straight-forward way by code that relies on\n> advanced compiler optimization to generate efficient code, otherwise\n> producing very suboptimal code.\n\nI don't know how difficult it was to arrive at the result above, but I \nwouldn't call inlining memcpy(3) an advanced optimization (anymore?), \nsince the major compilers seem to be doing that.\n\nThe SWAP in prio-queue.c seems to be the one with the biggest \nperformance impact. Or perhaps it's the one in lookup_object()? The \nformer is easier to measure, though.\n\nHere's what I get with CFLAGS=\"-builtin -O2\" (best of five):\n\n$ time ./t/helper/test-prio-queue $(seq 100000 -1 1) dump >/dev/null\n\nreal 0m0.142s\nuser 0m0.120s\nsys 0m0.020s\n\nAnd this is with CFLAGS=\"-no-builtin -O2\":\n\n$ time ./t/helper/test-prio-queue $(seq 100000 -1 1) dump >/dev/null\n\nreal 0m0.170s\nuser 0m0.156s\nsys 0m0.012s\n\nHmm. Not nice, but also not prohibitively slow.\n\n> The commit you quoted embarrasses me, and I have no excuse for it. I would\n> love to see that myswap() ugliness fixed by replacing it with a construct\n> that is simpler, and generates good code even without any smart compiler.\n\nI don't see a way to do that without adding a type parameter.\n\nRené\n"},{"id":"310566","messageId":"77098ac8-1084-a5c4-a5e6-fb382e3dc3a0@web.de","threadId":"44995","inReplyTo":"alpine.DEB.2.20.1701302158110.3469@virtualbox","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-01-30T22:09:23Z","receivedAt":"2017-01-30T22:10:26Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.01.2017 um 22:03 schrieb Johannes Schindelin:\n> It is curious, though, that an\n> expression like \"sizeof(a++)\" would not be rejected.\n\nClang normally warns about something like this (\"warning: expression \nwith side effects has no effect in an unevaluated context \n[-Wunevaluated-expression]\"), but not if the code is part of a macro. I \ndon't know if that's intended, but it sure is helpful in the case of SWAP.\n\n> Further, what would SWAP(a++, b) do? Swap a and b, and *then* increment a?\n\nThat might be a valid expectation, but GCC says \"error: lvalue required \nas unary '&' operand\" and clang puts it \"error: cannot take the address \nof an rvalue of type\".\n\nRené\n"},{"id":"310567","messageId":"20170130222157.GC35626@google.com","threadId":"44995","inReplyTo":"77098ac8-1084-a5c4-a5e6-fb382e3dc3a0@web.de","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-01-30T22:21:57Z","receivedAt":"2017-01-30T22:22:04Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 01/30, René Scharfe wrote:\n> Am 30.01.2017 um 22:03 schrieb Johannes Schindelin:\n> >It is curious, though, that an\n> >expression like \"sizeof(a++)\" would not be rejected.\n> \n> Clang normally warns about something like this (\"warning: expression\n> with side effects has no effect in an unevaluated context\n> [-Wunevaluated-expression]\"), but not if the code is part of a\n> macro. I don't know if that's intended, but it sure is helpful in\n> the case of SWAP.\n> \n> >Further, what would SWAP(a++, b) do? Swap a and b, and *then* increment a?\n> \n> That might be a valid expectation, but GCC says \"error: lvalue\n> required as unary '&' operand\" and clang puts it \"error: cannot take\n> the address of an rvalue of type\".\n> \n> René\n\nPerhaps we could disallow a side-effect operator in the macro. By\ndisallow I mean place a comment at the definition to the macro and\nhopefully catch something like that in code-review. We have the same\nissue with the `ALLOC_GROW()` macro.\n\n-- \nBrandon Williams\n"},{"id":"310568","messageId":"xmqqr33ktch9.fsf@gitster.mtv.corp.google.com","threadId":"44995","inReplyTo":"187c2b39-40cf-7e07-b489-d40cdf5f9145@web.de","subject":"Re: [PATCH 3/5] use SWAP macro","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-30T22:22:10Z","receivedAt":"2017-01-30T22:22:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> \t\tif (tree2->flags & UNINTERESTING) {\n> -\t\t\tstruct object *tmp = tree2;\n> -\t\t\ttree2 = tree1;\n> -\t\t\ttree1 = tmp;\n> +\t\t\tSWAP(tree2, tree1);\n\nA human would have written this SWAP(tree1, tree2).\n\nNot that I think such a manual fix-up should be made in _this_\npatch, which may end up mixing mechanical conversion (which we may\nwant to keep reproducible) and hand tweaks. But this swapped swap\nreads somewhat silly.\n\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index f420786039..1ae09894d7 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -186,9 +186,8 @@ static int queue_diff(struct diff_options *o,\n> \n> \t\tif (DIFF_OPT_TST(o, REVERSE_DIFF)) {\n> \t\t\tunsigned tmp;\n> -\t\t\tconst char *tmp_c;\n> \t\t\ttmp = mode1; mode1 = mode2; mode2 = tmp;\n> -\t\t\ttmp_c = name1; name1 = name2; name2 = tmp_c;\n> +\t\t\tSWAP(name1, name2);\n\nCurious that mode swapping is left for a later iteration.\n\n> diff --git a/diff.c b/diff.c\n> index f08cd8e033..9de1ba264f 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -5118,13 +5118,11 @@ void diff_change(struct diff_options *options,\n> \n> \tif (DIFF_OPT_TST(options, REVERSE_DIFF)) {\n> \t\tunsigned tmp;\n> -\t\tconst unsigned char *tmp_c;\n> -\t\ttmp = old_mode; old_mode = new_mode; new_mode = tmp;\n> -\t\ttmp_c = old_sha1; old_sha1 = new_sha1; new_sha1 = tmp_c;\n> +\t\tSWAP(old_mode, new_mode);\n> +\t\tSWAP(old_sha1, new_sha1);\n> \t\ttmp = old_sha1_valid; old_sha1_valid = new_sha1_valid;\n> \t\t\tnew_sha1_valid = tmp;\n\nSo is this one.\n\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> ...\n> -\t\t\ttmp = ren2;\n> -\t\t\tren2 = ren1;\n> -\t\t\tren1 = tmp;\n> +\t\t\tSWAP(ren2, ren1);\n\nA human would have written this SWAP(ren1, ren2).\n\n"},{"id":"310569","messageId":"xmqqmve8tcg2.fsf@gitster.mtv.corp.google.com","threadId":"44995","inReplyTo":"84944ecd-d14e-b5e9-7566-9ab2b68c02fb@web.de","subject":"Re: [PATCH 4/5] diff: use SWAP macro","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-30T22:22:53Z","receivedAt":"2017-01-30T22:23:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Use the macro SWAP to exchange the value of pairs of variables instead\n> of swapping them manually with the help of a temporary variable. The\n> resulting code is shorter and easier to read.\n>\n> The two cases were not transformed by the semantic patch swap.cocci\n> because it's extra careful and handles only cases where the types of all\n> variables are the same\n\nAh, that explains my \"huh?\" in 3/5; thanks.\n"},{"id":"310574","messageId":"xmqqzii8rv7y.fsf@gitster.mtv.corp.google.com","threadId":"44995","inReplyTo":"8ef4c833-45bd-6831-0683-6d01f30aa518@web.de","subject":"Re: [PATCH 0/5] introduce SWAP macro","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-30T23:20:17Z","receivedAt":"2017-01-30T23:20:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Exchanging the value of two variables requires declaring a temporary\n> variable and repeating their names. The swap macro in apply.c\n> simplifies this for its callers without changing the compiled binary.\n> Polish this macro and export it, then use it throughout the code to\n> reduce repetition and hide the declaration of temporary variables.\n\nAll looked reasonable (modulo \"swap tree2 and tree1???\" ordering).\nAlso the object-id ones looked good.\n\nThanks.\n"},{"id":"310598","messageId":"alpine.DEB.2.20.1701311302310.3469@virtualbox","threadId":"44995","inReplyTo":"77098ac8-1084-a5c4-a5e6-fb382e3dc3a0@web.de","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-31T12:03:55Z","receivedAt":"2017-01-31T12:05:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Mon, 30 Jan 2017, René Scharfe wrote:\n\n> Am 30.01.2017 um 22:03 schrieb Johannes Schindelin:\n> > It is curious, though, that an expression like \"sizeof(a++)\" would not\n> > be rejected.\n> \n> Clang normally warns about something like this (\"warning: expression\n> with side effects has no effect in an unevaluated context\n> [-Wunevaluated-expression]\"), but not if the code is part of a macro. I\n> don't know if that's intended, but it sure is helpful in the case of\n> SWAP.\n\nThank you for clarifying.\n\n> > Further, what would SWAP(a++, b) do? Swap a and b, and *then*\n> > increment a?\n> \n> That might be a valid expectation, but GCC says \"error: lvalue required\n> as unary '&' operand\" and clang puts it \"error: cannot take the address\n> of an rvalue of type\".\n\nOkay, now we know what the tool says.\n\nI would like to take a step back and state that it does not make much\nsense to support SWAP(a++, b), as it is confusing at best.\n\nCiao,\nDscho"},{"id":"310599","messageId":"alpine.DEB.2.20.1701311305030.3469@virtualbox","threadId":"44995","inReplyTo":"6760493c-3684-b8bb-2c01-6621b8417246@web.de","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-31T12:13:29Z","receivedAt":"2017-01-31T12:20:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Mon, 30 Jan 2017, René Scharfe wrote:\n\n> Am 30.01.2017 um 21:48 schrieb Johannes Schindelin:\n>\n> > The commit you quoted embarrasses me, and I have no excuse for it. I\n> > would love to see that myswap() ugliness fixed by replacing it with a\n> > construct that is simpler, and generates good code even without any\n> > smart compiler.\n> \n> I don't see a way to do that without adding a type parameter.\n\nExactly. And you know what? I would be very okay with that type parameter.\n\nCoccinelle [*1*] should be able to cope with that, too, mehtinks.\n\nIt would be trivially \"optimized\" out of the box, even when compiling with\nTiny C or in debug mode.\n\nAnd it would even allow things like this:\n\n#define SIMPLE_SWAP(T, a, b) do { T tmp_ = a; a = b; b = tmp_; } while (0)\n...\n\tuint32_t large;\n\tchar nybble;\n\n\t...\n\n\tif (!(large & ~0xf)) {\n\t\tSIMPLE_SWAP(char, nybble, large);\n\t\t...\n\t}\n\ni.e. mixing types, when possible.\n\nAnd while I do not necessarily expect that we need anything like this\nanytime soon, merely the fact that it allows for this flexibility, while\nbeing very readable at the same time, would make it a pretty good design\nin my book.\n\nCiao,\nDscho\n\nP.S.: I am sure glad they use the French name and do not translate it to\nEnglish."},{"id":"310617","messageId":"676ed19c-0c4e-341e-ba30-1f4a23440088@web.de","threadId":"44995","inReplyTo":"alpine.DEB.2.20.1701311305030.3469@virtualbox","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-01-31T21:02:54Z","receivedAt":"2017-01-31T21:03:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 31.01.2017 um 13:13 schrieb Johannes Schindelin:\n> Hi René,\n>\n> On Mon, 30 Jan 2017, René Scharfe wrote:\n>\n>> Am 30.01.2017 um 21:48 schrieb Johannes Schindelin:\n>>\n>>> The commit you quoted embarrasses me, and I have no excuse for it. I\n>>> would love to see that myswap() ugliness fixed by replacing it with a\n>>> construct that is simpler, and generates good code even without any\n>>> smart compiler.\n>>\n>> I don't see a way to do that without adding a type parameter.\n>\n> Exactly. And you know what? I would be very okay with that type parameter.\n>\n> Coccinelle [*1*] should be able to cope with that, too, mehtinks.\n\nYes, a semantic patch can turn the type of the temporary variable into a \nmacro parameter. Programmers would have to type the type, though, \nmaking the macro only half as good.\n\n> It would be trivially \"optimized\" out of the box, even when compiling with\n> Tiny C or in debug mode.\n\nSuch a compiler is already slowed down by memset(3) calls for \ninitializing objects and lack of other optimizations. I doubt a few \nmore memcpy(3) calls would make that much of a difference.\n\nNB: git as compiled with TCC fails several tests, alas. Builds wickedly \nfast, though.\n\n> And it would even allow things like this:\n>\n> #define SIMPLE_SWAP(T, a, b) do { T tmp_ = a; a = b; b = tmp_; } while (0)\n> ...\n> \tuint32_t large;\n> \tchar nybble;\n>\n> \t...\n>\n> \tif (!(large & ~0xf)) {\n> \t\tSIMPLE_SWAP(char, nybble, large);\n> \t\t...\n> \t}\n>\n> i.e. mixing types, when possible.\n>\n> And while I do not necessarily expect that we need anything like this\n> anytime soon, merely the fact that it allows for this flexibility, while\n> being very readable at the same time, would make it a pretty good design\n> in my book.\n\nSuch a skinny macro which only hides repetition is kind of attractive \ndue to its simplicity; I can't say the same about the mixed type example \nabove, though.\n\nThe fat version isn't that bad either even without inlining, includes a \nfew safety checks and doesn't require us to tell the compiler something \nit already knows very well. I'd rather let the machine do the work.\n\nRené\n"},{"id":"310618","messageId":"47dca1ce-14db-cba0-bbe0-9873fc0000b5@web.de","threadId":"44995","inReplyTo":"xmqqr33ktch9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/5] use SWAP macro","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-01-31T21:02:47Z","receivedAt":"2017-01-31T21:03:35Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.01.2017 um 23:22 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> \t\tif (tree2->flags & UNINTERESTING) {\n>> -\t\t\tstruct object *tmp = tree2;\n>> -\t\t\ttree2 = tree1;\n>> -\t\t\ttree1 = tmp;\n>> +\t\t\tSWAP(tree2, tree1);\n>\n> A human would have written this SWAP(tree1, tree2).\n>\n> Not that I think such a manual fix-up should be made in _this_\n> patch, which may end up mixing mechanical conversion (which we may\n> want to keep reproducible) and hand tweaks. But this swapped swap\n> reads somewhat silly.\n\nWell, a human wrote \"tmp = tree2\" -- sometimes one likes to count down \ninstead of up, I guess.\n\nWe can make Coccinelle order the parameters alphabetically, but I don't \nknow how to do so without duplicating most of the semantic patch. And \nthats would result in e.g. SWAP(new_tree, old_tree), which might be odd \nas well.\n\nI'd just leave them in whatever order they were, but that's probably \nbecause I'm lazy.\n\nRené\n"},{"id":"310619","messageId":"8e94756a-c3a5-9b81-268d-d0f36876f710@web.de","threadId":"44995","inReplyTo":"20170130222157.GC35626@google.com","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-01-31T21:03:01Z","receivedAt":"2017-01-31T21:03:48Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.01.2017 um 23:21 schrieb Brandon Williams:\n> On 01/30, René Scharfe wrote:\n>> Am 30.01.2017 um 22:03 schrieb Johannes Schindelin:\n>>> It is curious, though, that an\n>>> expression like \"sizeof(a++)\" would not be rejected.\n>>\n>> Clang normally warns about something like this (\"warning: expression\n>> with side effects has no effect in an unevaluated context\n>> [-Wunevaluated-expression]\"), but not if the code is part of a\n>> macro. I don't know if that's intended, but it sure is helpful in\n>> the case of SWAP.\n>>\n>>> Further, what would SWAP(a++, b) do? Swap a and b, and *then* increment a?\n>>\n>> That might be a valid expectation, but GCC says \"error: lvalue\n>> required as unary '&' operand\" and clang puts it \"error: cannot take\n>> the address of an rvalue of type\".\n>>\n>> René\n>\n> Perhaps we could disallow a side-effect operator in the macro. By\n> disallow I mean place a comment at the definition to the macro and\n> hopefully catch something like that in code-review. We have the same\n> issue with the `ALLOC_GROW()` macro.\n\nSWAP(a++, ...) is caught by the compiler, SWAP(*a++, ...) works fine. \nTechnically that should be enough. :) A comment wouldn't hurt, of course.\n\nRené\n"},{"id":"310620","messageId":"20170131213507.uiwmkkcg7umvd3f4@sigill.intra.peff.net","threadId":"44995","inReplyTo":"8e94756a-c3a5-9b81-268d-d0f36876f710@web.de","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-31T21:35:08Z","receivedAt":"2017-01-31T21:35:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 31, 2017 at 10:03:01PM +0100, René Scharfe wrote:\n\n> > Perhaps we could disallow a side-effect operator in the macro. By\n> > disallow I mean place a comment at the definition to the macro and\n> > hopefully catch something like that in code-review. We have the same\n> > issue with the `ALLOC_GROW()` macro.\n> \n> SWAP(a++, ...) is caught by the compiler, SWAP(*a++, ...) works fine.\n> Technically that should be enough. :) A comment wouldn't hurt, of course.\n\nOne funny thing is that your macro takes address-of itself, behind the\nscenes. I wonder if it would be more natural for it to take\npointers-to-objects, making it look more like a real function (i.e.,\nSWAP(&a, &b) instead of SWAP(a, b)\". And then these funny corner cases\nbecome quite obvious in the caller, because the caller is the one who\nhas to type \"&\".\n\n-Peff\n"},{"id":"310628","messageId":"xmqqy3xqq2w0.fsf@gitster.mtv.corp.google.com","threadId":"44995","inReplyTo":"20170131213507.uiwmkkcg7umvd3f4@sigill.intra.peff.net","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-31T22:29:51Z","receivedAt":"2017-01-31T22:30:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... I wonder if it would be more natural for it to take\n> pointers-to-objects, making it look more like a real function (i.e.,\n> SWAP(&a, &b) instead of SWAP(a, b)\". And then these funny corner cases\n> become quite obvious in the caller, because the caller is the one who\n> has to type \"&\".\n\nHmmmm. \n\nWhile this looks very attractive in theory by forcing 'a' and 'b' to\nbe lvalues, it probably invites mistakes go unnoticed during the\nreview when the code wants to swap two pointer variables. \n\nFor example,\n\napply.c: SWAP(p->new_name, p->old_name);\n\nis now a bug and will swap only the first byte of these names, and\nthe correct way to spell it would become:\n\napply.c: SWAP(&p->new_name, &p->old_name);\n\nThe latter clearly looks like swapping the new and old names, which\nis good, but I do not have any confidence that I will immediately\nspot a bug when presented the former under the new world order.\n"},{"id":"310629","messageId":"20170131223604.oa4h5ykhefvlergd@sigill.intra.peff.net","threadId":"44995","inReplyTo":"xmqqy3xqq2w0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-31T22:36:04Z","receivedAt":"2017-01-31T22:36:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 31, 2017 at 02:29:51PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > ... I wonder if it would be more natural for it to take\n> > pointers-to-objects, making it look more like a real function (i.e.,\n> > SWAP(&a, &b) instead of SWAP(a, b)\". And then these funny corner cases\n> > become quite obvious in the caller, because the caller is the one who\n> > has to type \"&\".\n> \n> Hmmmm. \n> \n> While this looks very attractive in theory by forcing 'a' and 'b' to\n> be lvalues, it probably invites mistakes go unnoticed during the\n> review when the code wants to swap two pointer variables. \n> \n> For example,\n> \n> apply.c: SWAP(p->new_name, p->old_name);\n> \n> is now a bug and will swap only the first byte of these names, and\n> the correct way to spell it would become:\n> \n> apply.c: SWAP(&p->new_name, &p->old_name);\n> \n> The latter clearly looks like swapping the new and old names, which\n> is good, but I do not have any confidence that I will immediately\n> spot a bug when presented the former under the new world order.\n\nYes, it's a problem with any function (or function-like interface) that\ntakes an untyped pointer. You don't know if the caller meant the pointer\nor the pointer-to-pointer. In that sense it's no different than:\n\n memcpy(p->new_name, p->old_name, len);\n\nIs that right, or did we mean to copy the pointers themselves?\n\nIt does also help the reverse case:\n\n int a, b;\n SWAP(a, b);\n\nwhich would give an error (you forgot the \"&\"). I don't know which is\nmore likely to be productive. But at least requiring pointer values\nmakes it consistent with other functions like memcpy, etc.\n\n-Peff\n"},{"id":"310633","messageId":"30d81b1c-1acc-4e4d-4f52-e056bbf7b3e2@ramsayjones.plus.com","threadId":"44995","inReplyTo":"676ed19c-0c4e-341e-ba30-1f4a23440088@web.de","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-02-01T00:44:02Z","receivedAt":"2017-02-01T00:44:10Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 31/01/17 21:02, René Scharfe wrote:\n[snip]\n>> It would be trivially \"optimized\" out of the box, even when compiling with\n>> Tiny C or in debug mode.\n> \n> Such a compiler is already slowed down by memset(3) calls for initializing objects and lack of other optimizations. I doubt a few more memcpy(3) calls would make that much of a difference.\n> \n> NB: git as compiled with TCC fails several tests, alas. Builds wickedly fast, though.\n\nHmm, a couple of years ago, I used tcc to build git and it worked\njust fine (and it passed all tests). Prompted by this note, I just\nbuilt tcc from the current mob branch (@5420bb8) and built git OK,\nbut, yes, lots of tests now fail. :(\n\nATB,\nRamsay Jones\n\n\n"},{"id":"310640","messageId":"alpine.DEB.2.20.1702011225250.3469@virtualbox","threadId":"44995","inReplyTo":"20170131213507.uiwmkkcg7umvd3f4@sigill.intra.peff.net","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-01T11:28:13Z","receivedAt":"2017-02-01T11:28:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 31 Jan 2017, Jeff King wrote:\n\n> On Tue, Jan 31, 2017 at 10:03:01PM +0100, René Scharfe wrote:\n> \n> > > Perhaps we could disallow a side-effect operator in the macro. By\n> > > disallow I mean place a comment at the definition to the macro and\n> > > hopefully catch something like that in code-review. We have the\n> > > same issue with the `ALLOC_GROW()` macro.\n> > \n> > SWAP(a++, ...) is caught by the compiler, SWAP(*a++, ...) works fine.\n> > Technically that should be enough. :) A comment wouldn't hurt, of\n> > course.\n> \n> One funny thing is that your macro takes address-of itself, behind the\n> scenes. I wonder if it would be more natural for it to take\n> pointers-to-objects, making it look more like a real function (i.e.,\n> SWAP(&a, &b) instead of SWAP(a, b)\". And then these funny corner cases\n> become quite obvious in the caller, because the caller is the one who\n> has to type \"&\".\n\nBut forcing SWAP(&a, &b) would make it even more cumbersome to use, and it\nwould also make it harder to optimize, say, by using registers instead of\naddressable memory (think swapping two 32-bit integers: there is *no* need\nto write them into memory just to swap them).\n\nAnd I think I should repeat my point that this discussion veers towards\nmaking simple swaps *more* complicated, rather than less complicated. Bad\ndirection.\n\nCiao,\nDscho"},{"id":"310641","messageId":"alpine.DEB.2.20.1702011228290.3469@virtualbox","threadId":"44995","inReplyTo":"676ed19c-0c4e-341e-ba30-1f4a23440088@web.de","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-01T11:39:15Z","receivedAt":"2017-02-01T11:39:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Tue, 31 Jan 2017, René Scharfe wrote:\n\n> Am 31.01.2017 um 13:13 schrieb Johannes Schindelin:\n>\n> > #define SIMPLE_SWAP(T, a, b) do { T tmp_ = a; a = b; b = tmp_; } while (0)\n> > ...\n> > uint32_t large;\n> > char nybble;\n> >\n> > ...\n> >\n> > if (!(large & ~0xf)) {\n> > SIMPLE_SWAP(char, nybble, large);\n> > ...\n> > }\n> >\n> > i.e. mixing types, when possible.\n> >\n> > And while I do not necessarily expect that we need anything like this\n> > anytime soon, merely the fact that it allows for this flexibility, while\n> > being very readable at the same time, would make it a pretty good design\n> > in my book.\n> \n> Such a skinny macro which only hides repetition is kind of attractive due to\n> its simplicity; I can't say the same about the mixed type example above,\n> though.\n> \n> The fat version isn't that bad either even without inlining, includes a few\n> safety checks and doesn't require us to tell the compiler something it\n> already knows very well. I'd rather let the machine do the work.\n\nI am a big fan of letting the machine do the work. But I am not a big fan\nof *creating* work for the machine.\n\nSo if you asked me what I would think of a script that, given a patch \"in\nmbox format\", automatically fixes all formatting issues that typically\ntake up a sizable chunk of review time, I would say: yeah, let's get this\ndone! It would probably take away some fun because then reviewers could\nbike-shed less, but I'd think that is a good thing.\n\nIf you asked me what my opinion is about a program you wrote that gathers\nall the threads and sub threads of code^Wpatch reviews on the Git mailing\nlist, and cross-references them with public Git branches, and with Junio's\nWhat's Cooking mail, and with the SHA-1s in `pu`: Great! That would\nrelieve me of a ton of really boring and grueling work, if the machine can\ndo it, all the better.\n\nAnd if you ask me about adding a complex macro that adds a bunch of work\nfor the C compiler just to produce the same assembler code as a one-liner\nmacro would have produced much easier, I will reply that we could maybe\ninstead spend that time on letting the machine perform tasks that already\nneed to be done... :-)\n\nCiao,\nDscho"},{"id":"310642","messageId":"20170201114750.r5xdy6emdczmnh4j@sigill.intra.peff.net","threadId":"44995","inReplyTo":"alpine.DEB.2.20.1702011225250.3469@virtualbox","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-01T11:47:51Z","receivedAt":"2017-02-01T11:48:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 01, 2017 at 12:28:13PM +0100, Johannes Schindelin wrote:\n\n> > One funny thing is that your macro takes address-of itself, behind the\n> > scenes. I wonder if it would be more natural for it to take\n> > pointers-to-objects, making it look more like a real function (i.e.,\n> > SWAP(&a, &b) instead of SWAP(a, b)\". And then these funny corner cases\n> > become quite obvious in the caller, because the caller is the one who\n> > has to type \"&\".\n> \n> But forcing SWAP(&a, &b) would make it even more cumbersome to use, and it\n> would also make it harder to optimize, say, by using registers instead of\n> addressable memory (think swapping two 32-bit integers: there is *no* need\n> to write them into memory just to swap them).\n\nI don't find the register thing compelling at all. I'd expect modern\ncompilers to optimize *(&a) into just \"a\" and keep using a register. I'm\nsure there are compilers that don't, but I'm also not sure how much we\n_care_. If your compiler doesn't do basic micro-optimizations, then you\nlive with it or get a better compiler.\n\nI'm much more interested in what's readable and maintainable, versus\ntrying to micro-optimize something that hasn't even been shown to be\nmeasurably interesting.\n\nThat said...\n\n> And I think I should repeat my point that this discussion veers towards\n> making simple swaps *more* complicated, rather than less complicated. Bad\n> direction.\n\nI'm not altogether convinced that SWAP() is an improvement in\nreadability. I really like that it's shorter than the code it replaces,\nbut there is a danger with introducing opaque constructs. It's one more\nthing for somebody familiar with C but new to the project to learn or\nget wrong.\n\nIMHO the main maintenance gain from René's patch is that you don't have\nto specify the type, which means you can never have a memory-overflow\nbug due to incorrect types. If we lose that, then I don't see all that\nmuch value in the whole thing.\n\n-Peff\n"},{"id":"310653","messageId":"bfd0d758-a9c8-9792-6294-9f9ed632cc98@web.de","threadId":"44995","inReplyTo":"20170201114750.r5xdy6emdczmnh4j@sigill.intra.peff.net","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-02-01T18:06:03Z","receivedAt":"2017-02-01T18:06:44Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.02.2017 um 12:47 schrieb Jeff King:\n> I'm not altogether convinced that SWAP() is an improvement in\n> readability. I really like that it's shorter than the code it replaces,\n> but there is a danger with introducing opaque constructs. It's one more\n> thing for somebody familiar with C but new to the project to learn or\n> get wrong.\n\nI'm biased, of course, but SIMPLE_SWAP and SWAP exchange the values of \ntwo variables -- how could someone get that wrong? Would it help to \nname the macro SWAP_VALUES? Or XCHG? ;)\n\n> IMHO the main maintenance gain from René's patch is that you don't have\n> to specify the type, which means you can never have a memory-overflow\n> bug due to incorrect types. If we lose that, then I don't see all that\n> much value in the whole thing.\n\nSize checks could be added to SIMPLE_SWAP as well.\n\nThe main benefit of a swap macro is reduced repetition IMHO: Users \nspecify the variables to swap once instead of twice in a special order, \nand with SWAP they don't need to declare their type again. Squeezing \nout redundancy makes the code easier to read and modify.\n\nRené\n"},{"id":"310654","messageId":"xmqqd1f1pxqc.fsf@gitster.mtv.corp.google.com","threadId":"44995","inReplyTo":"bfd0d758-a9c8-9792-6294-9f9ed632cc98@web.de","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-01T18:33:31Z","receivedAt":"2017-02-01T18:33:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Size checks could be added to SIMPLE_SWAP as well.\n\nBetween SWAP() and SIMPLE_SWAP() I am undecided.\n\nIf the compiler can infer the type and the size of the two\n\"locations\" given to the macro, there is no technical reason to\nrequire the caller to specify the type as an extra argument, so\nSIMPLE_SWAP() may not necessarily an improvement over SWAP() from\nthat point of view. If the redundancy is used as a sanity check,\nI'd be in favor of SIMPLE_SWAP(), though.\n\nIf the goal of SIMPLE_SWAP(), on the other hand, were to support the\n\"only swap char with int for small value\" example earlier in the\nthread, it means you cannot sanity check the type of things being\nswapped in the macro, and the readers of the code need to know about\nthe details of what variables are being swapped. It looks to me\nthat it defeats the main benefit of using a macro.\n\n> The main benefit of a swap macro is reduced repetition IMHO: Users\n> specify the variables to swap once instead of twice in a special\n> order, and with SWAP they don't need to declare their type again.\n> Squeezing out redundancy makes the code easier to read and modify.\n\nYes.\n"},{"id":"311011","messageId":"12e7db44-ff69-a38e-322a-6b5fc5f1fc29@web.de","threadId":"44995","inReplyTo":"xmqqd1f1pxqc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-02-07T22:04:21Z","receivedAt":"2017-02-07T22:04:39Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.02.2017 um 19:33 schrieb Junio C Hamano:\n> René Scharfe writes:\n> \n>> Size checks could be added to SIMPLE_SWAP as well.\n> \n> Between SWAP() and SIMPLE_SWAP() I am undecided.\n> \n> If the compiler can infer the type and the size of the two\n> \"locations\" given to the macro, there is no technical reason to\n> require the caller to specify the type as an extra argument, so\n> SIMPLE_SWAP() may not necessarily an improvement over SWAP() from\n> that point of view. If the redundancy is used as a sanity check,\n> I'd be in favor of SIMPLE_SWAP(), though.\n> \n> If the goal of SIMPLE_SWAP(), on the other hand, were to support the\n> \"only swap char with int for small value\" example earlier in the\n> thread, it means you cannot sanity check the type of things being\n> swapped in the macro, and the readers of the code need to know about\n> the details of what variables are being swapped. It looks to me\n> that it defeats the main benefit of using a macro.\n\nFull type inference could be done with C11's _Generic for basic types,\nwhile typeof would be needed for complex ones, I guess. Checking that\nsizes match is better than nothing and portable to ancient platforms,\nthough. Having an explicit type given is portable and easy to use for\nchecks, of course, e.g. like this:\n\n#define SIMPLE_SWAP(T, a, b) do { \\\n\tT swap_tmp_ = a + BUILD_ASSERT_OR_ZERO(sizeof(T) == sizeof(a)); \\\n\ta = b + BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b)); \\\n\tb = swap_tmp_; \\\n} while(0)\n\nIt doesn't support expressions with side effects, but that's probably\nnot much of a concern.\n\nSwapping between different types would then still have to be done\nmanually, but I wonder how common that is -- couldn't find such a case\nin our tree.\n\nRené\n"},{"id":"311012","messageId":"xmqqr339y6q3.fsf@gitster.mtv.corp.google.com","threadId":"44995","inReplyTo":"12e7db44-ff69-a38e-322a-6b5fc5f1fc29@web.de","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-07T22:30:12Z","receivedAt":"2017-02-07T22:30:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Swapping between different types would then still have to be done\n> manually, but I wonder how common that is -- couldn't find such a case\n> in our tree.\n\nI do not think it is a common thing to do, and more importantly, I\ndoubt we want to hide such a swap inside a macro. And that is why\nI said the seemingly extra \"type\" thing may be an improvement over\nyour original SWAP() thing if it gives us more type safety.\n\nIt seems that the thread has been quite for a while. Perhaps people\nare happy enough with your patches? If so, let's move it forward,\nbut I'll wait for a while in case follow-up discussion appears\nsoonish. The changes are fairly well isolated and I do not think we\nare in a hurry.\n\n\n"},{"id":"311064","messageId":"alpine.DEB.2.20.1702081612130.3496@virtualbox","threadId":"44995","inReplyTo":"xmqqr339y6q3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-08T15:14:54Z","receivedAt":"2017-02-08T15:47:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio & René,\n\nOn Tue, 7 Feb 2017, Junio C Hamano wrote:\n\n> René Scharfe writes:\n> \n> > Swapping between different types would then still have to be done\n> > manually, but I wonder how common that is -- couldn't find such a case\n> > in our tree.\n> \n> I do not think it is a common thing to do, and more importantly, I doubt\n> we want to hide such a swap inside a macro. And that is why I said the\n> seemingly extra \"type\" thing may be an improvement over your original\n> SWAP() thing if it gives us more type safety.\n> \n> It seems that the thread has been quite for a while. Perhaps people are\n> happy enough with your patches? If so, let's move it forward, but I'll\n> wait for a while in case follow-up discussion appears soonish. The\n> changes are fairly well isolated and I do not think we are in a hurry.\n\nI am still unhappy about choosing to complicate things and lean heavily on\nthe compiler to make things right again.\n\nBut it appears that I am the only one with this concern, so go ahead. I\npromise not to say \"I told you so\" in case that it breaks things or that\ncertain platforms are experiencing a disadvantage.\n\nCiao,\nJohannes"},{"id":"317670","messageId":"20170424112928.rty5xejep4mnxph2@sigill.intra.peff.net","threadId":"44995","inReplyTo":"0bdb58a6-3a7f-2218-4b70-c591ae90e95e@web.de","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-24T11:29:28Z","receivedAt":"2017-04-24T11:29:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 28, 2017 at 10:38:21PM +0100, René Scharfe wrote:\n\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 87237b092b..66cd466eea 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -527,6 +527,16 @@ static inline int ends_with(const char *str, const char *suffix)\n> \treturn strip_suffix(str, suffix, &len);\n> }\n> \n> +#define SWAP(a, b) do {\t\t\t\t\t\t\\\n> +\tvoid *_swap_a_ptr = &(a);\t\t\t\t\\\n> +\tvoid *_swap_b_ptr = &(b);\t\t\t\t\\\n> +\tunsigned char _swap_buffer[sizeof(a)];\t\t\t\\\n> +\tmemcpy(_swap_buffer, _swap_a_ptr, sizeof(a));\t\t\\\n> +\tmemcpy(_swap_a_ptr, _swap_b_ptr, sizeof(a) +\t\t\\\n> +\t BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b)));\t\\\n> +\tmemcpy(_swap_b_ptr, _swap_buffer, sizeof(a));\t\t\\\n> +} while (0)\n\nWhat should:\n\n SWAP(foo[i], foo[j]);\n\ndo when i == j? With this code, it ends up calling\n\n memcpy(&foo[i], &foo[j], ...);\n\nwhich can cause valgrind to complain about overlapping memory. I suspect\nin practice that noop copies are better off than partial overlaps, but I\nthink it does still violate the standard.\n\nIs it worth comparing the pointers and bailing early?\n\nA related question is whether the caller should ever be asking to swap\nsomething with itself. This particular case[1] comes from\nprio_queue_reverse(). I suspect its \"<=\" could become a \"<\", but I\nhaven't thought it through carefully.\n\n-Peff\n\n[1] http://public-inbox.org/git/CACsJy8AAtV5KJHBqWvnYb3Mw9CVzEdG3M-UJA+jd5MR5e-UMsA@mail.gmail.com/\n"},{"id":"317672","messageId":"20170424114919.kvgiybj2cm7xecpp@sigill.intra.peff.net","threadId":"44995","inReplyTo":"20170424112928.rty5xejep4mnxph2@sigill.intra.peff.net","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-24T11:49:20Z","receivedAt":"2017-04-24T11:49:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 24, 2017 at 07:29:28AM -0400, Jeff King wrote:\n\n> A related question is whether the caller should ever be asking to swap\n> something with itself. This particular case[1] comes from\n> prio_queue_reverse(). I suspect its \"<=\" could become a \"<\", but I\n> haven't thought it through carefully.\n\nOK, I actually looked closer and we definitely should drop the \"<=\". I\nwas thrown off by the assignment of \"j\" inside the loop condition. IMHO\nit might be more obvious to write as:\n\n for (i = 0, j = queue->nr - 1; i < j; i++, j--)\n\tswap(queue, i, j);\n\nbut I left it as-is.\n\nSo I think this patch is an obvious improvement whatever we do with\nSWAP(). There's still an open question of whether SWAP() should be more\ncareful about its inputs (normally I'd say yes, but I know this is a\npotentially performance critical call).\n\n-- >8 --\nSubject: [PATCH] prio_queue_reverse: don't swap elements with themselves\n\nOur array-reverse algorithm does the usual \"walk from both\nends, swapping elements\". We can quit when the two indices\nare equal, since:\n\n 1. Swapping an element with itself is a noop.\n\n 2. If i and j are equal, then in the next iteration i is\n guaranteed to be bigge than j, and we will exit the\n loop.\n\nSo exiting the loop on equality is slightly more efficient.\nAnd more importantly, the new SWAP() macro does not expect\nto handle noop swaps; it will call memcpy() with the same src\nand dst pointers in this case. It's unclear whether that\ncauses a problem on any platforms by violating the\n\"overlapping memory\" constraint of memcpy, but it does cause\nvalgrind to complain.\n\nSigned-off-by: Jeff King \n---\n prio-queue.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/prio-queue.c b/prio-queue.c\nindex 17252d231..fc3860fdc 100644\n--- a/prio-queue.c\n+++ b/prio-queue.c\n@@ -21,7 +21,7 @@ void prio_queue_reverse(struct prio_queue *queue)\n \n \tif (queue->compare != NULL)\n \t\tdie(\"BUG: prio_queue_reverse() on non-LIFO queue\");\n-\tfor (i = 0; i <= (j = (queue->nr - 1) - i); i++)\n+\tfor (i = 0; i < (j = (queue->nr - 1) - i); i++)\n \t\tswap(queue, i, j);\n }\n \n-- \n2.13.0.rc0.459.g06dc2b676\n\n"},{"id":"317677","messageId":"CACsJy8DoK__R5k0+5EVoagc+NiFNd1yjxWxwOS234H-wJ047wg@mail.gmail.com","threadId":"44995","inReplyTo":"20170424114919.kvgiybj2cm7xecpp@sigill.intra.peff.net","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-24T13:13:55Z","receivedAt":"2017-04-24T13:14:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Apr 24, 2017 at 6:49 PM, Jeff King wrote:\n> diff --git a/prio-queue.c b/prio-queue.c\n> index 17252d231..fc3860fdc 100644\n> --- a/prio-queue.c\n> +++ b/prio-queue.c\n> @@ -21,7 +21,7 @@ void prio_queue_reverse(struct prio_queue *queue)\n>\n> if (queue->compare != NULL)\n> die(\"BUG: prio_queue_reverse() on non-LIFO queue\");\n> - for (i = 0; i <= (j = (queue->nr - 1) - i); i++)\n> + for (i = 0; i < (j = (queue->nr - 1) - i); i++)\n> swap(queue, i, j);\n> }\n\nFWIW This shuts valgrind-3.10.1 up.\n-- \nDuy\n"},{"id":"318190","messageId":"070a4b85-98e0-12a4-6c9c-557f3dfa733c@web.de","threadId":"44995","inReplyTo":"20170424112928.rty5xejep4mnxph2@sigill.intra.peff.net","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-04-28T17:04:51Z","receivedAt":"2017-04-28T17:05:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 24.04.2017 um 13:29 schrieb Jeff King:\n> On Sat, Jan 28, 2017 at 10:38:21PM +0100, René Scharfe wrote:\n> \n>> diff --git a/git-compat-util.h b/git-compat-util.h\n>> index 87237b092b..66cd466eea 100644\n>> --- a/git-compat-util.h\n>> +++ b/git-compat-util.h\n>> @@ -527,6 +527,16 @@ static inline int ends_with(const char *str, const char *suffix)\n>> \treturn strip_suffix(str, suffix, &len);\n>> }\n>> \n>> +#define SWAP(a, b) do {\t\t\t\t\t\t\\\n>> +\tvoid *_swap_a_ptr = &(a);\t\t\t\t\\\n>> +\tvoid *_swap_b_ptr = &(b);\t\t\t\t\\\n>> +\tunsigned char _swap_buffer[sizeof(a)];\t\t\t\\\n>> +\tmemcpy(_swap_buffer, _swap_a_ptr, sizeof(a));\t\t\\\n>> +\tmemcpy(_swap_a_ptr, _swap_b_ptr, sizeof(a) +\t\t\\\n>> +\t BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b)));\t\\\n>> +\tmemcpy(_swap_b_ptr, _swap_buffer, sizeof(a));\t\t\\\n>> +} while (0)\n> \n> What should:\n> \n> SWAP(foo[i], foo[j]);\n> \n> do when i == j? With this code, it ends up calling\n> \n> memcpy(&foo[i], &foo[j], ...);\n> \n> which can cause valgrind to complain about overlapping memory. I suspect\n> in practice that noop copies are better off than partial overlaps, but I\n> think it does still violate the standard.\n> \n> Is it worth comparing the pointers and bailing early?\n\nHmm, so swapping a value with itself can be a useful thing to do?\nOtherwise an assert would be more appropriate.\n\nSwapping with *partial* overlap sounds tricky, or even evil. If\nwe want to support that for some reason we'd have to use memmove\nin the middle. But that would still corrupt at least one of the\nobjects, wouldn't it?\n\n> A related question is whether the caller should ever be asking to swap\n> something with itself. This particular case[1] comes from\n> prio_queue_reverse(). I suspect its \"<=\" could become a \"<\", but I\n> haven't thought it through carefully.\n\nThe line in question is this one:\n\n\tfor (i = 0; i <= (j = (queue->nr - 1) - i); i++)\n\nAssignment in the middle? Hmm. Why not do it like this?\n\n\tfor (i = 0, j = queue->nr - 1; i < j; i++, j--)\n\nLooks less complicated to me.\n\nRené\n"},{"id":"318211","messageId":"20170428214934.tuqihgch6qeen3ni@sigill.intra.peff.net","threadId":"44995","inReplyTo":"070a4b85-98e0-12a4-6c9c-557f3dfa733c@web.de","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-28T21:49:34Z","receivedAt":"2017-04-28T21:49:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 28, 2017 at 07:04:51PM +0200, René Scharfe wrote:\n\n> > What should:\n> > \n> > SWAP(foo[i], foo[j]);\n> > \n> > do when i == j? With this code, it ends up calling\n> > \n> > memcpy(&foo[i], &foo[j], ...);\n> > \n> > which can cause valgrind to complain about overlapping memory. I suspect\n> > in practice that noop copies are better off than partial overlaps, but I\n> > think it does still violate the standard.\n> > \n> > Is it worth comparing the pointers and bailing early?\n> \n> Hmm, so swapping a value with itself can be a useful thing to do?\n> Otherwise an assert would be more appropriate.\n\nNo, I doubt that it's useful, and it's probably a sign of a bug\nelsewhere. But it's likely a _harmless_ bug, so it may be irritating to\ndie when we hit it rather than continuing.\n\nI dunno. I could go either way. Or we could leave it as-is, and let\nvalgrind find the problem. That has zero run-time cost, but of course\nnobody bothers to run valgrind outside of the test suite, so the inputs\nare not usually very exotic.\n\n> Swapping with *partial* overlap sounds tricky, or even evil. If\n> we want to support that for some reason we'd have to use memmove\n> in the middle. But that would still corrupt at least one of the\n> objects, wouldn't it?\n\nYes, the overlap case is probably an actual bug. Detecting it is a bit\nharder, but definitely possible. I hate to pay the run-time cost for it,\nbut I wonder if a compiler could optimize it out.\n\n> The line in question is this one:\n> \n> \tfor (i = 0; i <= (j = (queue->nr - 1) - i); i++)\n> \n> Assignment in the middle? Hmm. Why not do it like this?\n> \n> \tfor (i = 0, j = queue->nr - 1; i < j; i++, j--)\n> \n> Looks less complicated to me.\n\nYes, see my other reply. :)\n\n-Peff\n"},{"id":"318242","messageId":"11699799-6bdf-484d-5a1c-8e8fa7981594@web.de","threadId":"44995","inReplyTo":"20170428214934.tuqihgch6qeen3ni@sigill.intra.peff.net","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-04-29T18:16:17Z","receivedAt":"2017-04-29T18:16:39Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 28.04.2017 um 23:49 schrieb Jeff King:\n> On Fri, Apr 28, 2017 at 07:04:51PM +0200, René Scharfe wrote:\n> \n>>> What should:\n>>>\n>>> SWAP(foo[i], foo[j]);\n>>>\n>>> do when i == j? With this code, it ends up calling\n>>>\n>>> memcpy(&foo[i], &foo[j], ...);\n>>>\n>>> which can cause valgrind to complain about overlapping memory. I suspect\n>>> in practice that noop copies are better off than partial overlaps, but I\n>>> think it does still violate the standard.\n>>>\n>>> Is it worth comparing the pointers and bailing early?\n>>\n>> Hmm, so swapping a value with itself can be a useful thing to do?\n>> Otherwise an assert would be more appropriate.\n> \n> No, I doubt that it's useful, and it's probably a sign of a bug\n> elsewhere. But it's likely a _harmless_ bug, so it may be irritating to\n> die when we hit it rather than continuing.\n> \n> I dunno. I could go either way. Or we could leave it as-is, and let\n> valgrind find the problem. That has zero run-time cost, but of course\n> nobody bothers to run valgrind outside of the test suite, so the inputs\n> are not usually very exotic.\n\nIt would be problematic on platforms where memcpy has to erase the\ndestination before writing new values (I don't know any example).\n\nWe could use two temporary buffers. The object code is the same with\nGCC around 5 and Clang 3.2 or higher -- at least for prio-queue.c.\n\nWith GCC 4.9.2 (that's what Debian stable currently has) the result is\nactually slightly better with two buffers because a 128-bit move starts\nto get used (https://godbolt.org/g/18HQDQ).\n\n>> Swapping with *partial* overlap sounds tricky, or even evil. If\n>> we want to support that for some reason we'd have to use memmove\n>> in the middle. But that would still corrupt at least one of the\n>> objects, wouldn't it?\n> \n> Yes, the overlap case is probably an actual bug. Detecting it is a bit\n> harder, but definitely possible. I hate to pay the run-time cost for it,\n> but I wonder if a compiler could optimize it out.\n\nHow is it possible to arrive at such a situation? We'd need two objects\nof the same size (we check that in SWAP) and one of them would start\ninside of the other one, i.e. the pointer difference between them would\nbe a fraction of 1. So the type system would have to be tricked into\nit, right?\n\nHow *would* we detect overlaps? The obvious checks (a+len<=b||b+len<=a)\nare undefined if applied to objects that don't belong to the same array.\nAnd members of the same array would not overlap to begin with..\n\nIt may be my laziness speaking, but do we really need such a check? If\nsomeone constructs interleaving objects then they'd need to implement\nthe necessary checks themselves IMHO.\n\n>> The line in question is this one:\n>>\n>> \tfor (i = 0; i <= (j = (queue->nr - 1) - i); i++)\n>>\n>> Assignment in the middle? Hmm. Why not do it like this?\n>>\n>> \tfor (i = 0, j = queue->nr - 1; i < j; i++, j--)\n>>\n>> Looks less complicated to me.\n> \n> Yes, see my other reply. :)\n\nAh, so that's where I stole it from. ;) Perhaps my source amnesia was\nin part caused by confusion about your reasoning there: The code does A,\nB would be better, so let's do C. Wait, what? :)\n\nRené\n"},{"id":"318251","messageId":"20170430031154.qurrbdb3wqrdxd37@sigill.intra.peff.net","threadId":"44995","inReplyTo":"11699799-6bdf-484d-5a1c-8e8fa7981594@web.de","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-30T03:11:55Z","receivedAt":"2017-04-30T03:12:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 29, 2017 at 08:16:17PM +0200, René Scharfe wrote:\n\n> > I dunno. I could go either way. Or we could leave it as-is, and let\n> > valgrind find the problem. That has zero run-time cost, but of course\n> > nobody bothers to run valgrind outside of the test suite, so the inputs\n> > are not usually very exotic.\n> \n> It would be problematic on platforms where memcpy has to erase the\n> destination before writing new values (I don't know any example).\n> \n> We could use two temporary buffers. The object code is the same with\n> GCC around 5 and Clang 3.2 or higher -- at least for prio-queue.c.\n\nHmm, yeah, that's an easy solution that covers the overlap case, too. If\nthe generated code is the same, that seems like it might not be bad (I\nam a little sad how complex this simple swap operation is getting,\nthough).\n\n> > Yes, the overlap case is probably an actual bug. Detecting it is a bit\n> > harder, but definitely possible. I hate to pay the run-time cost for it,\n> > but I wonder if a compiler could optimize it out.\n> \n> How is it possible to arrive at such a situation? We'd need two objects\n> of the same size (we check that in SWAP) and one of them would start\n> inside of the other one, i.e. the pointer difference between them would\n> be a fraction of 1. So the type system would have to be tricked into\n> it, right?\n\nYeah, I guess it would be pretty odd. I was thinking of swapping a\nstruct and one of its components, but that fail the size equality check.\nAnd anyway that would be a silly thing to do in the first place, so it's\nprobably not worth thinking too much about.\n\n> It may be my laziness speaking, but do we really need such a check? If\n> someone constructs interleaving objects then they'd need to implement\n> the necessary checks themselves IMHO.\n\nYeah, I think we can live without it. I was mostly trying to think\nthrough whether there were worse cases than the one we saw. But you've\nconvinced me that there probably aren't.\n\n> > > The line in question is this one:\n> > > \n> > > \tfor (i = 0; i <= (j = (queue->nr - 1) - i); i++)\n> > > \n> > > Assignment in the middle? Hmm. Why not do it like this?\n> > > \n> > > \tfor (i = 0, j = queue->nr - 1; i < j; i++, j--)\n> > > \n> > > Looks less complicated to me.\n> > \n> > Yes, see my other reply. :)\n> \n> Ah, so that's where I stole it from. ;) Perhaps my source amnesia was\n> in part caused by confusion about your reasoning there: The code does A,\n> B would be better, so let's do C. Wait, what? :)\n\nMy reasoning was that there's a bug, and the patch does the minimal fix.\nI don't mind making a readability improvement on top, but the two are\northogonal. I.e., you could still write:\n\n for (i = 0, j = queue->nr - 1; i <= j; i++, j--)\n\nwhich is more readable but still buggy. :)\n\n-Peff\n"},{"id":"318474","messageId":"57741b45-81b4-5940-3ffb-10c3b067cce3@web.de","threadId":"44995","inReplyTo":"20170430031154.qurrbdb3wqrdxd37@sigill.intra.peff.net","subject":"Re: [PATCH 1/5] add SWAP macro","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-05-02T05:29:35Z","receivedAt":"2017-05-02T05:29:58Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.04.2017 um 05:11 schrieb Jeff King:\n> On Sat, Apr 29, 2017 at 08:16:17PM +0200, René Scharfe wrote:\n> \n>>> I dunno. I could go either way. Or we could leave it as-is, and let\n>>> valgrind find the problem. That has zero run-time cost, but of course\n>>> nobody bothers to run valgrind outside of the test suite, so the inputs\n>>> are not usually very exotic.\n>>\n>> It would be problematic on platforms where memcpy has to erase the\n>> destination before writing new values (I don't know any example).\n>>\n>> We could use two temporary buffers. The object code is the same with\n>> GCC around 5 and Clang 3.2 or higher -- at least for prio-queue.c.\n> \n> Hmm, yeah, that's an easy solution that covers the overlap case, too. If\n> the generated code is the same, that seems like it might not be bad (I\n> am a little sad how complex this simple swap operation is getting,\n> though).\n\nPatch below.\n\nAll is well if the compiler can (and is allowed to) see through the\nmemcpy calls, otherwise we get a performance hit and this change makes\nthat slow path slower. FWIW, I didn't see any slowdown in our perf\ntests, though.\n\nIt's not too late to switch to a macro that takes a type parameter, as\nDscho suggested earlier:\n\n #define swap_t(T, a, b) do {T t_ = (a); (a) = (b); (b) = t_;} while (0)\n\nMuch simpler and with full type check, but harder to use (needs correct\ntype parameter and its other parameters must not have side effects).\n\n-- >8 --\nSubject: [PATCH] avoid calling memcpy on overlapping objects in SWAP\n\nCopy both objects into their own buffers before swapping to make sure we\ndon't call memcpy with overlapping memory ranges, as that's undefined.\nCompilers that inline the memcpy calls optimize the extra operations\naway. That allows calls like SWAP(x, x) without ill effect and without\nextra checks.\n\nSigned-off-by: Rene Scharfe \n---\n git-compat-util.h | 16 +++++++++-------\n 1 file changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex bd04564..a843f04 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -528,13 +528,15 @@ static inline int ends_with(const char *str, const char *suffix)\n }\n \n #define SWAP(a, b) do {\t\t\t\t\t\t\\\n-\tvoid *_swap_a_ptr = &(a);\t\t\t\t\\\n-\tvoid *_swap_b_ptr = &(b);\t\t\t\t\\\n-\tunsigned char _swap_buffer[sizeof(a)];\t\t\t\\\n-\tmemcpy(_swap_buffer, _swap_a_ptr, sizeof(a));\t\t\\\n-\tmemcpy(_swap_a_ptr, _swap_b_ptr, sizeof(a) +\t\t\\\n-\t BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b)));\t\\\n-\tmemcpy(_swap_b_ptr, _swap_buffer, sizeof(a));\t\t\\\n+\tvoid *swap_a_ptr_ = &(a);\t\t\t\t\\\n+\tvoid *swap_b_ptr_ = &(b);\t\t\t\t\\\n+\tunsigned char swap_a_buffer_[sizeof(a)];\t\t\\\n+\tunsigned char swap_b_buffer_[sizeof(a) +\t\t\\\n+\t\tBUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b))];\t\\\n+\tmemcpy(swap_a_buffer_, swap_a_ptr_, sizeof(a));\t\t\\\n+\tmemcpy(swap_b_buffer_, swap_b_ptr_, sizeof(a));\t\t\\\n+\tmemcpy(swap_a_ptr_, swap_b_buffer_, sizeof(a));\t\t\\\n+\tmemcpy(swap_b_ptr_, swap_a_buffer_, sizeof(a));\t\t\\\n } while (0)\n \n #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)\n-- \n2.1.4\n"}]}