On Monday 04 April 2005 13:41, Peter N. Lundblad wrote:
> And if you read that function, you see that itt will modify ch when it
> returns an error. Same holds for parse_section AFAICT.
I saw that.
> If there is a place
> where ch is not set to EOF, please show it or -even better - provide a
> patch to fix that.
Well, my proposed patch for escape sequences :-)
> The problem with your patch (ignoring the leak) is that it makes the code
> very confusing. In many (all I think) places, it sets both err and the
> character to EOF. Then there is a special test at the end of the loop.
Yes.
> > And from my feeling, the test should be for
> >
> > err != SVN_NO_ERROR,
> > not some other secondary variable, which is likely to be forgotten (as it
> > happened me with parsing escape-characters!)
>
> IMO this is a valid point. I don't know why the code is like it is, and
> there is no comment explaining it either. Also, parse_option and
> parse_section are poorly documented.
That's why I overlooked that point.
> Maybe the reason is performance, although I think this extra pointer check
> would hardly make a measurable difference, even in a parsing loop like
> this.
And all functions set err. So it doesn't make sense to set another variable to
a special value too, because testing a char is not faster than testing a
pointer.
> If you think this hsould be fixed, feel free to submit a complete patch
> which we could discuss.
>
> > > OTOH, looking at the code below, it seems that we leak an error if err
> > > got set in the loop and ferror returns true. It needs a deeper look,
> > > though.
> >
> > Ok.
> > Leaving it to you.
>
> Heh...
Well, I don't think I'll find the time this week. So feel free to do that :-)
Regards,
Phil
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Apr 4 15:01:46 2005