Re: [PATCH 3/5] diff --color-moved-ws: fix a memory leak

[Martin Ågren]

On Tue, 2 Oct 2018 at 20:04, Phillip Wood  wrote:
> const struct emitted_diff_symbol *longer =  a->len > b->len ? a : b;
> const struct emitted_diff_symbol *shorter = a->len > b->len ? b : a;
> int d = longer->len - shorter->len;
> +   int ret = !strncmp(longer->line + d, shorter->line, shorter->len);
> +
> +   if (!ret)
> +   return ret;
>
> out->string = xmemdupz(longer->line, d);
> out->current_longer = (a == longer);
>
> -   return !strncmp(longer->line + d, shorter->line, shorter->len);
> +   return ret;
>  }

The caller only cares about 0 vs non-0, so this would also work:

   if (strncmp(...))
   return 0;

   ...

   return 1;

Just a thought, to avoid some "!"s and the new variable `ret`.

Martin

Re: [PATCH 3/5] diff --color-moved-ws: fix a memory leak

[Phillip Wood]

On 02/10/2018 20:08, Stefan Beller wrote:
> On Tue, Oct 2, 2018 at 10:55 AM Phillip Wood  
> wrote:
>>
>> From: Phillip Wood 
>>
>> Don't duplicate the indentation string if we're not going to use it.
>> This was found with asan.
> 
> Makes sense,
> 
> Thanks,
> Stefan
> 
> With compute_ws_delta growing bigger here (and having only one caller),
> I wonder if we want to pass 'match' instead of match->es (and pmb_nr),
> such that the function can also take care of
> pmb[pmb_nr++].match = match;

Wouldn't we still need to increment pmb_nr in the caller though, in
which case I'm not sure it saves much.

> Then the function is not a mere compute_ws_delta, but a whole
> setup_new_pmb(...) thing. Undecided.
> 

Re: [PATCH 3/5] diff --color-moved-ws: fix a memory leak

[Stefan Beller]

On Tue, Oct 2, 2018 at 10:55 AM Phillip Wood  wrote:
>
> From: Phillip Wood 
>
> Don't duplicate the indentation string if we're not going to use it.
> This was found with asan.

Makes sense,

Thanks,
Stefan

With compute_ws_delta growing bigger here (and having only one caller),
I wonder if we want to pass 'match' instead of match->es (and pmb_nr),
such that the function can also take care of
pmb[pmb_nr++].match = match;

Then the function is not a mere compute_ws_delta, but a whole
setup_new_pmb(...) thing. Undecided.

[PATCH 3/5] diff --color-moved-ws: fix a memory leak

[Phillip Wood]

From: Phillip Wood 

Don't duplicate the indentation string if we're not going to use it.
This was found with asan.

Signed-off-by: Phillip Wood 
---
 diff.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 0096bdc339..efadd05c90 100644
--- a/diff.c
+++ b/diff.c
@@ -785,11 +785,15 @@ static int compute_ws_delta(const struct 
emitted_diff_symbol *a,
const struct emitted_diff_symbol *longer =  a->len > b->len ? a : b;
const struct emitted_diff_symbol *shorter = a->len > b->len ? b : a;
int d = longer->len - shorter->len;
+   int ret = !strncmp(longer->line + d, shorter->line, shorter->len);
+
+   if (!ret)
+   return ret;
 
out->string = xmemdupz(longer->line, d);
out->current_longer = (a == longer);
 
-   return !strncmp(longer->line + d, shorter->line, shorter->len);
+   return ret;
 }
 
 static int cmp_in_block_with_wsd(const struct diff_options *o,
-- 
2.19.0