Potential AV introduced by r2518

[GoogleCodeExporter]

A check for Owner=nil is missing from TdwsJSONValue.TElementEnumerator.MoveNext.

Also, if MoveNext returns False then the internal state should not be modified 
(i.e. don't alter Index).

Original issue reported on code.google.com by anders.b...@gmail.com on 17 Apr 2014 at 4:42

[GoogleCodeExporter]

Original comment by zar...@gmail.com on 18 Apr 2014 at 1:28

[GoogleCodeExporter]

Original comment by zar...@gmail.com on 18 Apr 2014 at 1:28

• Changed state: Accepted

[GoogleCodeExporter]

Added tests and checked code, there is no issue.

- TdwsJSONValue is meant to allow operation on nil instances (cf. 
http://www.delphitools.info/2012/09/17/spotlight-on-dwsjson/)
- made the FIndex field private, further increases to the Index field have no 
effect on public state (adding code to keep it unmodified would only server to 
affect performance negatively)

Original comment by zar...@gmail.com on 23 Apr 2014 at 7:49

• Changed state: Done

[GoogleCodeExporter]

You are correct; There is no issue.
I was not aware of this feature. It would be nice if it was documented in the 
wiki instead of an external page.
Wrt. to incrementing the Index I was only concerned about the hypothetic 
situation where repeated calls to MoveNext, after False had been returned, 
would eventually cause the value to roll over and become negative. I don't 
think the performance price would be significant, but I haven't checked the 
generated asm.

Original comment by anders.b...@gmail.com on 23 Apr 2014 at 11:01

[GoogleCodeExporter]

Thanks for clarification. I couldn't see an issue here either, which is why I 
skipped it for the moment, but forgot to check it later.

Anyway, nice that you added a test and some improvements, as I added this code 
a bit too impulsively.

Original comment by CWBudde on 23 Apr 2014 at 1:20

[GoogleCodeExporter]

Did some quick tests, on a simple loop, with a an array of 1000 elements:

   for i := 0 to a.ElementCount-1 do
      e := a.Elements[i];

vs

   for e in a do ;

And I also added inlining for the GetEnumerator, and made branch-less MoveNext 
with check.

"For to" loop completes in 220, "for in" without MoveNext check completes in 
750, "for in" with MoveNext check completes in 800.

So all in all, the GetEnumerator overhead is at least 300%, so a case can be 
made that optimizing GetEnumerator is irrelevant anyway, since you shouldn't be 
using it in performance-critical code.

I've committed the branch-less code.

Original comment by zar...@gmail.com on 23 Apr 2014 at 3:41

[GoogleCodeExporter]

Committed an extra optimization that takes "for in" to 620.

(since that GetEnumerator is in order, it can't handle mutations, so might as 
well enforce the count to be static for the duration of the enumeration)

Original comment by zar...@gmail.com on 23 Apr 2014 at 4:01