<p>> On 02.11.2015 20:24, Sven Barth wrote:<br>
>>><br>
>>> The only thing we need is a new flag/whatever so that <br>
>>> *create_for_in_loop* knows that "tcallnode(expr)" contains an enumerator <br>
>>> function. tnode.flags seems to be full :( Do you have any suggestions? <br>
>>> If we sort this out, 2 more arguments against it are nil :) <br>
>><br>
>><br>
>> I see three possibilities to avoid the addition of a global flag: <br>
>> - simply always return the enumerator function if no arguments are given for an indexed array (pro: easy to implement; con: will bite us once we add another functionality that works on the array property as a whole) <br>
><br>
><br>
> Yes, this is not very clean. Furthermore such code would compile as well:<br>
><br>
> x := MyObj.MyArrayProperty;<br>
><br>
> (x would get the enumerator function, which is wrong.)</p>
<p>Not if we define it as the intended purpose. But also note that I had mentioned in my "con" that defining it like this might bite us layer on if we have a different usecase for code like the above. So I don't favor that option either anyway.</p>
<p>><br>
>> - always return the enumerator, but add checks everywhere except the for-in parsing against the enumerator (maybe for this case the enumerator node would be an advantage) (pro: enumerator will only work in for-in; con: every expression handling code needs to maintain that code) <br>
><br>
><br>
> Looks unnecessarily complicated.</p>
<p>That's my opinion as well ;)</p>
<p>><br>
>> - extend comp_expr by another boolean parameter (which is set in for_in_loop_create) and pass that down to factor (even better: convert the boolean parameters of comp_expr, sub_expr and factor to a set, would be cleaner anyway, IMHO) (pro: the state is only maintained locally and new flags can be added easily; con: a greater change in the compiler, though that would be a onetime thing)<br>
><br>
><br>
> Looks unnecessarily complicated for me as well.</p>
<p>And here we disagree. The "inforin" you added to tscannerfile is something we don't want to see introduced (especially since we're trying to reduce reliance on such (pseudo)global state) and the only way to fix this is by passing down the flag through comp_expr to handle_propertysym.</p>
<p>> ---<br>
><br>
> From my POV, having thought about the problem for some days already, the use of tenumeratornode is the simplest and clearest way to achieve the goal. The property array enumerator use in for-in is a "single case" only. So the use of a single-purpose node is clear as well - everybody understands on the first sight what is going on.</p>
<p>No, I think using a tcallnode is a more powerful approach. create_for_in_loop only needs to check that the tcallnode returns a valid enumerator type (namely one containing "Current" and "MoveNext") and be done with it. This way one can easily extend the for-in with other functionality if the need arises.</p>
<p>><br>
> It is way more dangerous and unclear if you use some flags or parameters that change the tcallnode handling. If you use tenumeratornode you clearly see in what code parts it is used and how. If you use tcallnode + some flag/parameter it will be devil. For such reasons there was OOP invented, so we should take advantage of it.</p>
<p>I'm not talking about the handling of tcallnode I'm talking about when handle_propertysym is returning the enumerator or not (completely independent of the usage of tenumeratornode or tcallnode).</p>
<p>> If you don't want to include the "enumeratorn" into tnodetype and the "is" operator from my first proposal is too slow, you can also directly type check for tenumeratornode in create_for_in_loop (in the case you don't allow tenumeratornode ancestors, of course):</p>
<p>This has nothing to do with the discussion we're currently having. If a new node would be introduced it would have to be implemented exactly like it's done with the other nodes, namely using nodetype and potentially being extendable. There are reasons for this approach after all.<br>
However I'm still not of the opinion that the enumerator node is needed at all.</p>
<p>Regards,<br>
Sven</p>