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
