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

From: Jeff King <peff@peff.net>
Date: Jan 24, 2019, 21:17 UTC
Message-ID: <20190124211715.GB16114@sigill.intra.peff.net>
In-Reply-To: <CAGZ79kZR--VN2BiG=usyy=J=u59Up3XYpK4c9pJYQkX1Gby06Q@mail.gmail.com>

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

Show 24 quoted lines

> >  - 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.

Show 10 quoted lines

> >  - 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