Re: [PATCH 4/5] diff: use SWAP macro

[Junio C Hamano]

René Scharfe  writes:

> Use the macro SWAP to exchange the value of pairs of variables instead
> of swapping them manually with the help of a temporary variable.  The
> resulting code is shorter and easier to read.
>
> The two cases were not transformed by the semantic patch swap.cocci
> because it's extra careful and handles only cases where the types of all
> variables are the same

Ah, that explains my "huh?" in 3/5; thanks.

Re: [PATCH 4/5] diff: use SWAP macro

[René Scharfe]

Am 30.01.2017 um 17:04 schrieb Johannes Schindelin:

Hi René,

On Sat, 28 Jan 2017, René Scharfe wrote:


Use the macro SWAP to exchange the value of pairs of variables instead
of swapping them manually with the help of a temporary variable.  The
resulting code is shorter and easier to read.

The two cases were not transformed by the semantic patch swap.cocci
because it's extra careful and handles only cases where the types of all
variables are the same -- and here we swap two ints and use an unsigned
temporary variable for that.  Nevertheless the conversion is safe, as
the value range is preserved with and without the patch.


One way to make this more obvious would be to change the type to signed
first, and then transform (which then would catch these cases too,
right?).


I'm not sure it would be more obvious, but it would certainly make the 
type change more explicit.  In diff-index.c we might even want to change 
the type of the swapped values from int to unsigned, which is more 
fitting for file modes.  In diff.c we'd need to add a separate variable, 
as tmp is shared with other (unsigned) swaps.


René

Re: [PATCH 4/5] diff: use SWAP macro

[Johannes Schindelin]

Hi René,

On Sat, 28 Jan 2017, René Scharfe wrote:

> Use the macro SWAP to exchange the value of pairs of variables instead
> of swapping them manually with the help of a temporary variable.  The
> resulting code is shorter and easier to read.
> 
> The two cases were not transformed by the semantic patch swap.cocci
> because it's extra careful and handles only cases where the types of all
> variables are the same -- and here we swap two ints and use an unsigned
> temporary variable for that.  Nevertheless the conversion is safe, as
> the value range is preserved with and without the patch.

One way to make this more obvious would be to change the type to signed
first, and then transform (which then would catch these cases too,
right?).

Ciao,
Dscho

[PATCH 4/5] diff: use SWAP macro

[René Scharfe]

Use the macro SWAP to exchange the value of pairs of variables instead
of swapping them manually with the help of a temporary variable.  The
resulting code is shorter and easier to read.

The two cases were not transformed by the semantic patch swap.cocci
because it's extra careful and handles only cases where the types of all
variables are the same -- and here we swap two ints and use an unsigned
temporary variable for that.  Nevertheless the conversion is safe, as
the value range is preserved with and without the patch.

Signed-off-by: Rene Scharfe 
---
 diff-no-index.c | 3 +--
 diff.c  | 4 +---
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/diff-no-index.c b/diff-no-index.c
index 1ae09894d7..df762fd0f7 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -185,8 +185,7 @@ static int queue_diff(struct diff_options *o,
struct diff_filespec *d1, *d2;
 
if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
-   unsigned tmp;
-   tmp = mode1; mode1 = mode2; mode2 = tmp;
+   SWAP(mode1, mode2);
SWAP(name1, name2);
}
 
diff --git a/diff.c b/diff.c
index 9de1ba264f..6c4f3f6b72 100644
--- a/diff.c
+++ b/diff.c
@@ -5117,11 +5117,9 @@ void diff_change(struct diff_options *options,
return;
 
if (DIFF_OPT_TST(options, REVERSE_DIFF)) {
-   unsigned tmp;
SWAP(old_mode, new_mode);
SWAP(old_sha1, new_sha1);
-   tmp = old_sha1_valid; old_sha1_valid = new_sha1_valid;
-   new_sha1_valid = tmp;
+   SWAP(old_sha1_valid, new_sha1_valid);
SWAP(old_dirty_submodule, new_dirty_submodule);
}
 
-- 
2.11.0