[Mono-devel-list] [Patch] RReader not to rely on bounds checking

[Willibald Krenn]

> Well, with your patch you would have never realized that bc wasn't
> working. Then somebody else would have spent that time debugging
> whatever error this bug could induce.

Me wonders why there's no testcase for that.. Me wonders why no-one else 
ever spotted this - the AMD64 port is quite 'old' now..

>>The exception will always be thrown, because in my rev. of 'mcs' code, 
>>the Parser reads in until RReader.Read returns -1..
> 
> 
> I don't think that one exception out of 16.000 calls is a problem.

The exception per se is no problem. But why going that route, if it's 
that simple to avoid it completely?

> I'm sorry you lost that time, but this doesn't mean that the original
> code was incorrect. 

It is incorrect because the catch should read
} catch (System.IndexOutOfRangeException) {

If that was the case, debugging would've been a lot easier.

> What if the ++ operator stops working? do we need to
> add a check for this as well in the Read() code? We don't need to. We
> can write code assuming that ++ will work, as well as we can assume that
> array bounds check will work. If any of those stop working somebody will
> have a hard time debugging it, but it doesn't mean that code written
> with those assumptions is wrong.

Code written with those assumptions is probably bad style, but not 
incorrect - I agree with that. What makes RReader.Read incorrect is the 
catch filter in combination with the bc dependency.

> I didn't say I don't like the patch. I said that you can commit it. I
> agree in that it's more elegant. 

I'll check abcremoval first. Unfortunately it seems the abcremover can't 
handle this case (even  return index < string.Length ? string[index] : 
-1) seems to be a problem.

Ultimate goal would be to eliminate the bounds check completely..

Willi