Re: [PATCH 1/5] add SWAP macro

Am 30.04.2017 um 05:11 schrieb Jeff King: > On Sat, Apr 29, 2017 at 08:16:17PM +0200, René Scharfe wrote: > >>> I dunno. I could go either way. Or we could leave it as-is, and let >>> valgrind find the problem. That has zero run-time cost, but of course >>> nobody bothers to run valgrind outside

Re: [PATCH 1/5] add SWAP macro

On Sat, Apr 29, 2017 at 08:16:17PM +0200, René Scharfe wrote: > > I dunno. I could go either way. Or we could leave it as-is, and let > > valgrind find the problem. That has zero run-time cost, but of course > > nobody bothers to run valgrind outside of the test suite, so the inputs > > are not

Re: [PATCH 1/5] add SWAP macro

Am 28.04.2017 um 23:49 schrieb Jeff King: On Fri, Apr 28, 2017 at 07:04:51PM +0200, René Scharfe wrote: What should: SWAP(foo[i], foo[j]); do when i == j? With this code, it ends up calling memcpy([i], [j], ...); which can cause valgrind to complain about overlapping memory. I

Re: [PATCH 1/5] add SWAP macro

On Fri, Apr 28, 2017 at 07:04:51PM +0200, René Scharfe wrote: > > What should: > > > >SWAP(foo[i], foo[j]); > > > > do when i == j? With this code, it ends up calling > > > >memcpy([i], [j], ...); > > > > which can cause valgrind to complain about overlapping memory. I suspect > > in

Re: [PATCH 1/5] add SWAP macro

Am 24.04.2017 um 13:29 schrieb Jeff King: On Sat, Jan 28, 2017 at 10:38:21PM +0100, René Scharfe wrote: diff --git a/git-compat-util.h b/git-compat-util.h index 87237b092b..66cd466eea 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -527,6 +527,16 @@ static inline int ends_with(const

Re: [PATCH 1/5] add SWAP macro

On Mon, Apr 24, 2017 at 6:49 PM, Jeff King wrote: > diff --git a/prio-queue.c b/prio-queue.c > index 17252d231..fc3860fdc 100644 > --- a/prio-queue.c > +++ b/prio-queue.c > @@ -21,7 +21,7 @@ void prio_queue_reverse(struct prio_queue *queue) > > if (queue->compare != NULL) >

Re: [PATCH 1/5] add SWAP macro

On Mon, Apr 24, 2017 at 07:29:28AM -0400, Jeff King wrote: > A related question is whether the caller should ever be asking to swap > something with itself. This particular case[1] comes from > prio_queue_reverse(). I suspect its "<=" could become a "<", but I > haven't thought it through

Re: [PATCH 1/5] add SWAP macro

On Sat, Jan 28, 2017 at 10:38:21PM +0100, René Scharfe wrote: > diff --git a/git-compat-util.h b/git-compat-util.h > index 87237b092b..66cd466eea 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -527,6 +527,16 @@ static inline int ends_with(const char *str, const char > *suffix) >

Re: [PATCH 1/5] add SWAP macro

Hi Junio & René, On Tue, 7 Feb 2017, Junio C Hamano wrote: > René Scharfe writes: > > > Swapping between different types would then still have to be done > > manually, but I wonder how common that is -- couldn't find such a case > > in our tree. > > I do not think it is a common

Re: [PATCH 1/5] add SWAP macro

René Scharfe writes: > Swapping between different types would then still have to be done > manually, but I wonder how common that is -- couldn't find such a case > in our tree. I do not think it is a common thing to do, and more importantly, I doubt we want to hide such a swap

Re: [PATCH 1/5] add SWAP macro

Am 01.02.2017 um 19:33 schrieb Junio C Hamano: > René Scharfe writes: > >> Size checks could be added to SIMPLE_SWAP as well. > > Between SWAP() and SIMPLE_SWAP() I am undecided. > > If the compiler can infer the type and the size of the two > "locations" given to the macro,

Re: [PATCH 1/5] add SWAP macro

René Scharfe writes: > Size checks could be added to SIMPLE_SWAP as well. Between SWAP() and SIMPLE_SWAP() I am undecided. If the compiler can infer the type and the size of the two "locations" given to the macro, there is no technical reason to require the caller to specify the

Re: [PATCH 1/5] add SWAP macro

Am 01.02.2017 um 12:47 schrieb Jeff King: I'm not altogether convinced that SWAP() is an improvement in readability. I really like that it's shorter than the code it replaces, but there is a danger with introducing opaque constructs. It's one more thing for somebody familiar with C but new to

Re: [PATCH 1/5] add SWAP macro

On Wed, Feb 01, 2017 at 12:28:13PM +0100, Johannes Schindelin wrote: > > One funny thing is that your macro takes address-of itself, behind the > > scenes. I wonder if it would be more natural for it to take > > pointers-to-objects, making it look more like a real function (i.e., > > SWAP(, )

Re: [PATCH 1/5] add SWAP macro

Hi René, On Tue, 31 Jan 2017, René Scharfe wrote: > Am 31.01.2017 um 13:13 schrieb Johannes Schindelin: > > > #define SIMPLE_SWAP(T, a, b) do { T tmp_ = a; a = b; b = tmp_; } while (0) > > ... > > uint32_t large; > > char nybble; > > > > ... > > > > if (!(large & ~0xf)) { > >

Re: [PATCH 1/5] add SWAP macro

Hi Peff, On Tue, 31 Jan 2017, Jeff King wrote: > On Tue, Jan 31, 2017 at 10:03:01PM +0100, René Scharfe wrote: > > > > Perhaps we could disallow a side-effect operator in the macro. By > > > disallow I mean place a comment at the definition to the macro and > > > hopefully catch something like

Re: [PATCH 1/5] add SWAP macro

On 31/01/17 21:02, René Scharfe wrote: [snip] >> It would be trivially "optimized" out of the box, even when compiling with >> Tiny C or in debug mode. > > Such a compiler is already slowed down by memset(3) calls for initializing > objects and lack of other optimizations. I doubt a few more

Re: [PATCH 1/5] add SWAP macro

On Tue, Jan 31, 2017 at 02:29:51PM -0800, Junio C Hamano wrote: > Jeff King writes: > > > ... I wonder if it would be more natural for it to take > > pointers-to-objects, making it look more like a real function (i.e., > > SWAP(, ) instead of SWAP(a, b)". And then these funny

Re: [PATCH 1/5] add SWAP macro

Jeff King writes: > ... I wonder if it would be more natural for it to take > pointers-to-objects, making it look more like a real function (i.e., > SWAP(, ) instead of SWAP(a, b)". And then these funny corner cases > become quite obvious in the caller, because the caller is the

Re: [PATCH 1/5] add SWAP macro

On Tue, Jan 31, 2017 at 10:03:01PM +0100, René Scharfe wrote: > > Perhaps we could disallow a side-effect operator in the macro. By > > disallow I mean place a comment at the definition to the macro and > > hopefully catch something like that in code-review. We have the same > > issue with the

Re: [PATCH 1/5] add SWAP macro

Am 30.01.2017 um 23:21 schrieb Brandon Williams: On 01/30, René Scharfe wrote: Am 30.01.2017 um 22:03 schrieb Johannes Schindelin: It is curious, though, that an expression like "sizeof(a++)" would not be rejected. Clang normally warns about something like this ("warning: expression with

Re: [PATCH 1/5] add SWAP macro

Am 31.01.2017 um 13:13 schrieb Johannes Schindelin: Hi René, On Mon, 30 Jan 2017, René Scharfe wrote: Am 30.01.2017 um 21:48 schrieb Johannes Schindelin: The commit you quoted embarrasses me, and I have no excuse for it. I would love to see that myswap() ugliness fixed by replacing it with

Re: [PATCH 1/5] add SWAP macro

Hi René, On Mon, 30 Jan 2017, René Scharfe wrote: > Am 30.01.2017 um 21:48 schrieb Johannes Schindelin: > > > The commit you quoted embarrasses me, and I have no excuse for it. I > > would love to see that myswap() ugliness fixed by replacing it with a > > construct that is simpler, and

Re: [PATCH 1/5] add SWAP macro

Hi René, On Mon, 30 Jan 2017, René Scharfe wrote: > Am 30.01.2017 um 22:03 schrieb Johannes Schindelin: > > It is curious, though, that an expression like "sizeof(a++)" would not > > be rejected. > > Clang normally warns about something like this ("warning: expression > with side effects has no

Re: [PATCH 1/5] add SWAP macro

On 01/30, René Scharfe wrote: > Am 30.01.2017 um 22:03 schrieb Johannes Schindelin: > >It is curious, though, that an > >expression like "sizeof(a++)" would not be rejected. > > Clang normally warns about something like this ("warning: expression > with side effects has no effect in an

Re: [PATCH 1/5] add SWAP macro

Am 30.01.2017 um 22:03 schrieb Johannes Schindelin: It is curious, though, that an expression like "sizeof(a++)" would not be rejected. Clang normally warns about something like this ("warning: expression with side effects has no effect in an unevaluated context [-Wunevaluated-expression]"),

Re: [PATCH 1/5] add SWAP macro

Am 30.01.2017 um 21:48 schrieb Johannes Schindelin: So I tried to verify that Visual C optimizes this well, and oh my deity, this was not easy. In Debug mode, it does not optimize, i.e. the memcpy() will be called, even for simple 32-bit integers. In Release mode, Visual Studio's defaults turn

Re: [PATCH 1/5] add SWAP macro

Hi Hannes, On Mon, 30 Jan 2017, Johannes Sixt wrote: > Am 30.01.2017 um 17:01 schrieb Johannes > Schindelin: > > On Sat, 28 Jan 2017, René Scharfe wrote: > > > diff --git a/git-compat-util.h > > > b/git-compat-util.h > > > index 87237b092b..66cd466eea 100644 > > > --- a/git-compat-util.h > > >

Re: [PATCH 1/5] add SWAP macro

Hi René, On Mon, 30 Jan 2017, René Scharfe wrote: > Am 30.01.2017 um 16:39 schrieb Johannes Schindelin: > > > On Sat, 28 Jan 2017, René Scharfe wrote: > > > > > Add a macro for exchanging the values of variables. It allows users > > > to avoid repetition and takes care of the temporary variable

Re: [PATCH 1/5] add SWAP macro

Am 30.01.2017 um 17:01 schrieb Johannes Schindelin: On Sat, 28 Jan 2017, René Scharfe wrote: diff --git a/git-compat-util.h b/git-compat-util.h index 87237b092b..66cd466eea 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -527,6 +527,16 @@ static inline int ends_with(const char *str,

Re: [PATCH 1/5] add SWAP macro

Am 30.01.2017 um 17:01 schrieb Johannes Schindelin: Hi René, On Sat, 28 Jan 2017, René Scharfe wrote: diff --git a/git-compat-util.h b/git-compat-util.h index 87237b092b..66cd466eea 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -527,6 +527,16 @@ static inline int ends_with(const

Re: [PATCH 1/5] add SWAP macro

Am 30.01.2017 um 16:39 schrieb Johannes Schindelin: Hi René, On Sat, 28 Jan 2017, René Scharfe wrote: Add a macro for exchanging the values of variables. It allows users to avoid repetition and takes care of the temporary variable for them. It also makes sure that the storage sizes of its

Re: [PATCH 1/5] add SWAP macro

Hi René, On Sat, 28 Jan 2017, René Scharfe wrote: > diff --git a/git-compat-util.h b/git-compat-util.h > index 87237b092b..66cd466eea 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -527,6 +527,16 @@ static inline int ends_with(const char *str, const char > *suffix) >

Re: [PATCH 1/5] add SWAP macro

Hi René, On Sat, 28 Jan 2017, René Scharfe wrote: > Add a macro for exchanging the values of variables. It allows users to > avoid repetition and takes care of the temporary variable for them. It > also makes sure that the storage sizes of its two parameters are the > same. Its memcpy(1)

[PATCH 1/5] add SWAP macro

Add a macro for exchanging the values of variables. It allows users to avoid repetition and takes care of the temporary variable for them. It also makes sure that the storage sizes of its two parameters are the same. Its memcpy(1) calls are optimized away by current compilers. Also add a