Enhanced looping tags for WML

[tekelili]

I am not proffessional coder, so my opinion is not relevant, just giving it for the sake of add points of view:

I hate intructions especificaly created to break loops, they pretty much remember me the classic "goto 10".
Usually loops can be breaken without ad hoc tags, by carefull manipulation of index included in loop condition. In my experience a tag to break loop would "ease" task too much and will lead to bad programings habits and very difficult to read code. Having break loop as an uncomfortable task is a good thing imo, because "force" coder to think how build them correctly.

[Pentarctagon]

I am not proffessional coder, so my opinion is not relevant, just giving it for the sake of add points of view:

I hate intructions especificaly created to break loops, they pretty much remember me the classic "goto 10".
Usually loops can be breaken without ad hoc tags, by carefull manipulation of index included in loop condition. In my experience a tag to break loop would "ease" task too much and will lead to bad programings habits and very difficult to read code. Having break loop as an uncomfortable task is a good thing imo, because "force" coder to think how build them correctly.

I'm not sure how it really eases the task much, since you can already create a {BREAK index} macro that works by setting the index to 2 billion or some huge number. While technically it could potentially not work, any case where it wouldn't work already represents a huge issue for other reasons.

[tekelili]

@Pentarctagon: What I pointed is mostly a trivial issue, but in some sense point a taste in design: Conditions under a loop will keep running would be always present as the very first keys in order to ease reading; and beign strict, if that loop can be broken by conditions different from [while] keys then it can be pointed as dirty code. I agree sometimes you may need this dirty hack, but I find create a tag specifically for a dirty coding tactic as not desirable.

[Celtic_Minstrel]

Hmmm. I think this might be a good idea, but I'm not quite sure how to go about it. I'll give it some thought though.

One possible way to go may be this one:

• At the start of the tag, when collecting the variables, you set a local Lua variable with the value of array_name.lenght
• Inside the outer for cycle, after the if not cfg.readonly block, you add another if check
• In this if check, you get again the value of array_name.length; if this value isn't the same as the one stored in the Lua variable, use helper.wml_error to raise and exit

This kind of check should be done at last, because the user may legitimately want to remove one element from the array, then use [break_if] to exit the cycle without doing any damage. In this case, the new if check should take a look to the return value of handle_event_commands as well.
Sounds good for a start?

I put the check right at the start of the loop, so it happens (redundantly) before they get a chance to do anything and then before each cycle starts. The check will still pass though if they add and delete the same number of elements during iteration.

I hate intructions especificaly created to break loops, they pretty much remember me the classic "goto 10".
Usually loops can be breaken without ad hoc tags, by carefull manipulation of index included in loop condition. In my experience a tag to break loop would "ease" task too much and will lead to bad programings habits and very difficult to read code. Having break loop as an uncomfortable task is a good thing imo, because "force" coder to think how build them correctly.

Firstly, using break greatly reduces the nesting level required. It does not share the dangers of goto, since where the break jumps to is determined entirely by the scope in which it appears. And finally, break as I implemented it cannot be used in a nested scope. (Well, it can, but it won't work as you expect.) So, you may still need to rearrange the logic a little to be able to use it.

I'm not sure how it really eases the task much, since you can already create a {BREAK index} macro that works by setting the index to 2 billion or some huge number. While technically it could potentially not work, any case where it wouldn't work already represents a huge issue for other reasons.

Though, that macro would have to be used with care since it doesn't prevent anything after it from executing.

[Pentarctagon]

I'm not sure how it really eases the task much, since you can already create a {BREAK index} macro that works by setting the index to 2 billion or some huge number. While technically it could potentially not work, any case where it wouldn't work already represents a huge issue for other reasons.

Though, that macro would have to be used with care since it doesn't prevent anything after it from executing.

It's functionally the exact opposite of how the [break_if] tag would work - rather than being evaluated first in the current interation it's evaluated last (or rather, at the start of the next iteration after it was used).

[Celtic_Minstrel]

That's true if you're referring to the implementation of [break_if] that Elvish Hunter posted. My own implementation can be evaluated anywhere in the while tag (ie, at the point where it appears, it's evaluated immediately and may or may not terminate processing of the loop) as long as it's not nested in another tag (like [if] or [switch] for example). If you want to you can view my implementation here.

[Pentarctagon]

One thing that I think should probably be mentioned somewhere is that by changing the NEXT macro to have [/for] instead of [/while], any user code that uses NEXT with [while] will break. For example I use NEXT to close this macro:

Code: Select all

#define FOREACH_SUBSET VAR START END

{VARIABLE {VAR} {START}}
[while]
  [variable]
    name={VAR}
    less_than={END}
  [/variable]
  [do]
#enddef

[Elvish_Hunter]

There's a possible issue that I found with [for].

Code: Select all

	else
		first = cfg.start or 0
		last = cfg["end"] or first
		step = cfg.step or ((last - first) / math.abs(last - first))
	end
	if ((last - first) / math.abs(last - first)) ~= (step / math.abs(step)) then
		-- Sanity check: If they specify something like start,end,step=1,4,-1
		-- then we interpret it as start,end,step=4,1,-1
		-- (The step takes precedence since it's optional.)
		last, first = first, last
	end

Let's imagine for a moment that someone used start, stop, step = 0, 0, 0.
In this case, (last - first) / math.abs(last - first)) doesn't equal to 1 or -1, like one may expect, but to "-nan" or "nan", as does step/math.abs(step). However, even if both will equal to nan, testing two nan values for equality always return false.
There's also another problem with step: there's no check if it equals to 0, and as such it may lead to an infinite loop - don't forget that in Lua, the only values that cast as false are false and nil, everything else (empty tables, empty strings, zeros...) is true.
In fact, both these chunks of code lead to infinite loops:

Code: Select all

    [event]
    	name=start
    	[for]
    		start, end, step = 0, 0, 1
    		[do]
    			[set_variable]
    				name=test
    				value=$i
    			[/set_variable]
    		[/do]
    	[/for]
    [/event]

Code: Select all

    [event]
    	name=start
    	[for]
    		start, end, step = 0, 10, 0
    		[do]
    			[set_variable]
    				name=test
    				value=$i
    			[/set_variable]
    		[/do]
    	[/for]
    [/event]

My suggestions are to add two guards to the code: the first one will issue a warning and exit the tag (or raise an error, you call) if step == 0; the second one will set an upvalue and use the loop_condition() closure (which seems to be responsible for the first chunk malfunctioning) to increase it, and will return false if the upvalue becomes greater than 2^16 - that is, allow at most 65536 iterations like [while] does.

[Celtic_Minstrel]

Note that, as currently implemented, the range of [for] includes both start and end - so for example, start,end=0,0 should mean the body is executed once. I personally think this makes more sense, but I'd like to hear from WML authors about this as several of the developers think it would make more sense for the range to include start but exclude end (so start,end=0,0 doesn't execute the body at all, while start,end=0,1 executes it once).

Regarding the issues EH reported, the first is easily fixable (and I have already done so in my branch), but the second fix depends on the above decision.

[tekelili]

Note that, as currently implemented, the range of [for] includes both start and end - so for example, start,end=0,0 should mean the body is executed once. I personally think this makes more sense, but I'd like to hear from WML authors about this as several of the developers think it would make more sense for the range to include start but exclude end (so start,end=0,0 doesn't execute the body at all, while start,end=0,1 executes it once).

Imo start= an end= should always mean initial and final values of "counter variable". I could be very easy be interested in do just one iteration with "counter variable" having "0" as value, so start,end=0,0 should do 1 iteration.

However I understand this can be unhandly sometimes. In case someone prefer "read" tag as from here do so many times, then key should be named (or added) as "times=", but no "end="

Edit: using start,end= should be able to do zero terations, and I think start=0,end=$null (or start=$null,end=99) should do such thing.
Edit2: thinking about it... have support for both keys end= and times= (mutualy exclusive) would be really usefull for coders imho, saving often embarrashing conversions to call macros with right argument.

[Pentarctagon]

2. Next, a for-range loop: [for]. This is actually intended to work as a drop-in replacement for the {FOREACH} macro, but could also be used for running over any arbitrary range.

I'm not sure if this is intentional or not, but if this is supposed to be a drop-in replacement for FOREACH then it needs to re-get the length of the array it's looping over each time through. For example, currently if I have an array of 12 and then remove an element mid loop it still loops 12 times rather than 11.

[Celtic_Minstrel]

Hmm, my thoughts on fixing this is to make it re-evaluate the loop bounds on every iteration, rather tan just evaluating them once at the start. However, that could result in the loop changing direction midway, which seems like a bad idea. For example:

Code: Select all

{VARIABLE start 1}
{VARIABLE end 5}
{VARIABLE step 1}
[for]
    start=$start
    end=$end
    step=$step
    [do]
        [chat]
            message="We are on loop $i!"
        [/chat]
        [if]
            {VARIABLE_COMPARISON i equals 3}
            [then]
                {VARIABLE start 5}
                {VARIABLE end 1}
                {VARIABLE step -1}
            [/then]
        [/if]
    [/do]
[/for]

With my proposed change, the above code would print the following to the chat area:

Code: Select all

We are on loop 1!
We are on loop 2!
We are on loop 3!
We are on loop 2!
We are on loop 1!

One alternative would be that attempts to change the direction of the loop cause it to terminate instead. Thus the code above would count up to 3 and then stop. Another alternative would be to only re-evaluate the bounds, not the step, and swap the bounds if they imply a direction change. With this alternative, the code above would count up to 5 and stop.

[Pentarctagon]

If someone is intentionally manipulating the variables responsible for controlling the loop while it's still looping, then it's really up to the person coding to make sure their logic is sound. Allowing that kind of control would in some ways even be a feature.

[Celtic_Minstrel]

I had this completed awhile ago but just now submitted the pull request, if you want to take a look. I chose not to re-evaluate the start point of the loop since it's logical to only use it once. I expect to merge this within a couple of days.

[Pentarctagon]

On a completely unrelated note, perhaps it would make sense to add macros for [break]/[continue]/[return] since they have no arguments? So instead of

Code: Select all

[break][/break]

it would be

Code: Select all

{BREAK}