Re: [PATCH 2/6] diff: clear emitted_symbols flag after use

[Jeff King <peff@peff.net>]

On Thu, Jan 24, 2019 at 12:36:38PM -0800, Stefan Beller wrote:

> >  - In an if() block we enter when o->emitted_symbols is set, there
> >    is a check to see if o->color_moved is set.  This makes sense
> >    only if we are trying to be prepared to handle a case where we
> >    are not the one that assigned a non-NULL to o->emitted_symbols
> >    due to o->color_moved.  So it certainly is possible that
> >    o->emitted_symbols is set before we enter this function.
> 
> Ah I see where you are coming from, as the code was written
> I imagined:
> 
>     if (o->color_moved)
>         o->emitted_symbols = &esm;
>     if (o->distim_gostaks)
>         o->emitted_symbols = &esm;
> 
>     if (o->emitted_symbols) {
>          if (o->color_moved)
>             handle_color_moved(o);
>         if (o->distim_gostaks)
>             handle_distimming(o);
> 
>         ... flush symbols...
>         ... free &cleanup ...
>     }

Yeah, FWIW this is what I took to be the reason for the code being laid
out as it is.

> >  - But then, it means that o->emitted_symbols we may have had
> >    non-NULL when the function is called may be overwritten if
> >    o->color_moved is set.
> 
> I see. So either we'd want to have
> 
>     if (o->emitted_symbols)
>         BUG("should not be already set");
> 
> or as Peff points out, make it non-static.

Even if it's non-static, you'd still want to ensure that it's not set
coming into the function (because somebody like diff-words might have
left it set, confusing us). So I think that BUG() may be worth having
either way.

-Peff