On Mon, 4 Apr 2005, Ph. Marek wrote:
> On Monday 04 April 2005 11:47, Peter N. Lundblad wrote:
> > This leaks the open file. also note that whenever err is set, ch gets set
> > to EOF, so the loop will terminate in that case. So, you don't need this
> > fix.
> No, it doesn't. The call to parse_option has no ch=EOF, and I'm not sure if
> it's set everywhere.
> else
> err = parse_option (&ch, &ctx);
> break;
And if you read that function, you see that itt will modify ch when it
returns an error. Same holds for parse_section AFAICT. If there is a place
where ch is not set to EOF, please show it or -even better - provide a
patch to fix that.
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.
> 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.
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.
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...
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Apr 4 13:37:51 2005