[elseif] in WML

[Elvish_Hunter]

A long time ago, I was quite surprised by the [if] [else] structure as it is used in WML. Coming from Python, I expected to find something like [elif], [elseif], or whatever you want to call it. After all, even other languages have such keyword, like Lua having elseif, Ruby having elsif, and Perl, Java and C++ being able to use "else if" on a single line. But not WML. Sure, we have [switch] [case], but this cannot substitute [elseif] in every possible instance - for example, what if we need to check a set of two or three variables?

I tried implementing it on my own some time ago, but I had to leave it, as I faced some issues, like having the [then] belonging to [if] executed when an [elseif] matched...
Recently, however, I attempted it again, and I managed to make something that seems to work. Of course, we're near to a new feature freeze, so I won't commit it for now. Consider it a proposal for the 1.13 series
That said, this is the tag that I concocted (I still need to test it a lot more, but for now it seems to work well):

Code: Select all

-- this function removes all the sub-tags from a given tag
-- returns the "cleaned" tag as literal and all its children
local function extract_children( lit_cfg , tag )
	local children = {}
	local new_cfg = lit_cfg

	for index=1, #new_cfg do
		if new_cfg[index][1] == tag then
			table.insert( children, new_cfg[index] )
		end
	end
	for index=#new_cfg, 1, -1 do
		if new_cfg[index][1] == tag then
			table.remove( new_cfg, index ) -- iterate backwards to remove items
		end
	end
	return new_cfg, children
end

local function if_handler_bis( cfg )
	-- [elseif] tags will contain conditions as well, so we need to remove them
	-- otherwise they'll interfere with [if]
	local then_children
	local elseif_children
	local else_children
	local new_cfg = helper.literal( cfg )
	new_cfg, then_children = extract_children(new_cfg, "then") -- insulate [then]
	new_cfg, elseif_children = extract_children(new_cfg, "elseif") -- insulate [elseif]
	new_cfg, else_children = extract_children(new_cfg, "else") -- insulate [else]

	if #then_children < 1 then -- we don't have even a single [then] ?
		helper.wml_error "[if] missing required [then] tag"
		return
	end

	local passed = wesnoth.eval_conditional( new_cfg ) -- we'll evalutate an [if] tag that contains only the conditions

	if passed then
		for then_child in helper.child_range ( then_children, "then" ) do
			handle_event_commands( then_child )
		end
		return -- stop after executing [then]
	else
		for elseif_child in helper.child_range ( elseif_children, "elseif" ) do
			if wesnoth.eval_conditional( elseif_child ) then -- we'll evalutate [elseif] tag
				-- there's no point in executing [elseif] without [then]
				local then_test = helper.get_child( elseif_child, "then" )
				if not then_test then
					helper.wml_error "[elseif] missing required [then] tag"
					return
				end
				-- test passed, proceed
				for then_tag in helper.child_range( elseif_child, "then" ) do
					handle_event_commands( then_tag ) -- it contains both condition
				return end -- stop on first matched condition
			end
		end
		-- no matched condition, try the [else] tags
		for else_child in helper.child_range ( else_children, "else" ) do
			handle_event_commands( else_child )
		end
	end
end

wml_actions["if"] = if_handler_bis

As you can see, I tried to keep backwards compatibility - and hopefully I managed to do so. Breaking backwards compatibility on a tag as widely used as [if] isn't a joke - and anyway I'll have to modify wmllint to warn about nested [if]s that may potentially be converted to [elseif].
That said, my modification adds two features:
1) of course, adds [elseif] support. [elseif] is used exactly as nowadays [if] is, but doesn't support [else] - it relies on the parent's [else] instead
2) in case that an [if] or [elseif] tag is missing its [then] tag, an explicit error is thrown, instead of failing silently.

Now, what are the pros and cons of my proposal? The pros are pretty much evident, as its use leads to a much cleaner code. See this block before:

Code: Select all

		[if]
			[variable]
				name=Grog_alive
				equals=yes
			[/variable]
			[variable]
				name=Elyssa_alive
				equals=yes
			[/variable]
			[then]
				[message]
					speaker=narrator
					message= _ "Both are alive"
				[/message]
			[/then]
			[else]
				[if]
					[variable]
						name=Grog_alive
						equals=yes
					[/variable]
					[variable]
						name=Elyssa_alive
						equals=no
					[/variable]
					[then]
						[message]
							speaker=narrator
							message= _ "Only Grog is alive"
						[/message]
					[/then]
					[else]
						[if]
							[variable]
								name=Grog_alive
								equals=no
							[/variable]
							[variable]
								name=Elyssa_alive
								equals=yes
							[/variable]
							[then]
								[message]
									speaker=narrator
									message= _ "Only Elyssa is alive"
								[/message]
							[/then]
							[else]
								[message]
									speaker=narrator
									message= _ "Nobody is alive"
								[/message]
							[/else]
						[/if]
					[/else]
				[/if]
			[/else]
		[/if]

and after:

Code: Select all

		[if]
			[variable]
				name=Grog_alive
				equals=yes
			[/variable]
			[variable]
				name=Elyssa_alive
				equals=yes
			[/variable]
			[then]
				[message]
					speaker=narrator
					message= _ "Both are alive"
				[/message]
			[/then]
			[elseif]
				[variable]
					name=Grog_alive
					equals=yes
				[/variable]
				[variable]
					name=Elyssa_alive
					equals=no
				[/variable]
				[then]
					[message]
						speaker=narrator
						message= _ "Only Grog is alive"
					[/message]
				[/then]
			[/elseif]
			[elseif]
				[variable]
					name=Grog_alive
					equals=no
				[/variable]
				[variable]
					name=Elyssa_alive
					equals=yes
				[/variable]
				[then]
					[message]
						speaker=narrator
						message= _ "Only Elyssa is alive"
					[/message]
				[/then]
			[/elseif]
			[else]
				[message]
					speaker=narrator
					message= _ "Nobody is alive"
				[/message]
			[/else]
		[/if]

It's evident that the second chunk is much more readable. And a readable code means less coding errors.
However, there is the opposite face of this medal, and it's that [if] is now MUCH slower. Take this block for example:

Code: Select all

		{VARIABLE counter 0}
		{VARIABLE_OP start time stamp}
		[while]
			[variable]
				name=counter
				less_than=65536
			[/variable]
			[do]
				[if]
					[variable]
						name=counter
						less_than=100000
					[/variable]
					[then]
						{VARIABLE_OP counter add 1}
					[/then]
				[/if]
			[/do]
		[/while]
		{VARIABLE_OP end time stamp}
		[message]
			speaker=narrator
			message="Elapsed time: $($end-$start)"
		[/message]

Executing three times the same block with the old if_handler returned me the following results:
5244 5149 5118
Using the new handler:
7760 7789 7810
In fact, if my calculations are correct, the new tag is slower by 50%! It should also be noted that I never seen 65000 [if] checks in a row, so in normal add-ons the speed loss shouldn't be noticeable.
So, the question is: are UMC authors interested in such WML improvement? Are the increased functionality and better code layout worth the performance loss? Do you see any place where I can improve the code? And above all, should I commit it once the feature freeze will be over (that means, next year) or not?

[Anonymissimus]

I'd say "yes". Sounds thought out.

A good way of cutting down such conditions into pieces with lower indentation level is using custom events with [filter_condition] though. Looking at your example code, these could be 4 events which are all tried to be executed and which check in their [filter_condition]s for the 4 variable combinations.
Also, why are you checking for the 4 possible combinations of conditions AB ? You could check A alone and both in that [then] and [else] check B. (I hope that makes sense...)

[8680]

I support this proposal. There are, however, some issues I would raise with it.

⁂

Firstly, re the implementation — and usage — of the extract_children function —

Code: Select all

-- this function removes all the sub-tags from a given tag
-- returns the "cleaned" tag as literal and all its children
local function extract_children( lit_cfg , tag )
	local children = {}
	local new_cfg = lit_cfg

	for index=1, #new_cfg do
		if new_cfg[index][1] == tag then
			table.insert( children, new_cfg[index] )
		end
	end
	for index=#new_cfg, 1, -1 do
		if new_cfg[index][1] == tag then
			table.remove( new_cfg, index ) -- iterate backwards to remove items
		end
	end
	return new_cfg, children
end

local new_cfg = lit_cfg — This appears to be written under the assumption that this will create a new table containing a copy of lit_cfg’s contents.

Such an assumption would be incorrect; this would in actuality copy only a pointer to lit_cfg, so new_cfg and lit_cfg will both refer to the same table object, so the mutations to new_cfg would also affect lit_cfg.

Given how extract_children is used —

Code: Select all

	new_cfg, then_children = extract_children(new_cfg, "then") -- insulate [then]
	new_cfg, elseif_children = extract_children(new_cfg, "elseif") -- insulate [elseif]
	new_cfg, else_children = extract_children(new_cfg, "else") -- insulate [else]

— this bug would not be apparent, as it would not alter the effect of these particular call-sites, which happen to be its only call-sites.

I would write extract_children thus —

Code: Select all

local function extract_children(cfg, tag_name)
	-- Removes all subtags with the given `tag_name` from the given `cfg`,
	-- and returns a list of the removed subtags.

	local subtags = {}
	
	for i = #cfg, 1, -1 do
		-- ^ Iterate backward for faster element removal.
		-- (Though for any quantities of subtags except the
		-- particularly excessive, the difference should be
		-- negligible.)
		if cfg[i][1] == tag_name then
			table.insert(subtags, 0, table.remove(cfg, i))
			-- Prepending the elements to the list like this (i.e.,
			-- inserting them at index 0) carries the cost of
			-- shifting all other elements up by one index, but,
			-- again, the cost should be negligible for typical
			-- quantities of subtags.
		end
	end
end

— and the first paragraph (so to speak) of if_handler_bis thus —

Code: Select all

local function if_handler_bis(cfg)
	cfg = helper.literal(cfg)
	local then_children = extract_children(cfg, 'then')
	local elseif_children = extract_children(cfg, 'elseif')
	local else_children = extract_children(cfg, 'else')

⁂

Secondly, you twice return after a helper.wml_error call. helper.wml_error immediately aborts down the call stack to the first protected (engine or pcall) function call, so a subsequent return is pointless — it will never be executed, except perhaps if the engine or its state is corrupt.

⁂

Thirdly, you raise an error if there is no [then] subtag in [if] or [elseif] — but

1. if there are multiple [then] or [else] subtags in [if], you execute the contents of all of them; and
2. if there are multiple [then] subtags in [elseif], you execute the first’s contents and silently ignore the rest.

What is the use of allowing multiple [then] or [else] subtags in [if], and why the inconsistency with [elseif]? (I suspect a typo in the latter case, given the anomalous return end.)

Why not raise an error if there are

1. multiple [then] subtags in [if] or [elseif], or
2. multiple [else] subtags in [if]?

⁂

Fourthly and finally, you use, e.g., helper.child_range ( then_children, "then" ) — why filter for [then] subtags, when then_children can contain nothing else (unless it is corrupt)?

[Elvish_Hunter]

Excuse me for the delay in answering.

I'd say "yes". Sounds thought out.

I support this proposal.

Thanks!

A good way of cutting down such conditions into pieces with lower indentation level is using custom events with [filter_condition] though. Looking at your example code, these could be 4 events which are all tried to be executed and which check in their [filter_condition]s for the 4 variable combinations.

Yes. Or one can use [switch][case]. It pretty much depends on the situation.

Also, why are you checking for the 4 possible combinations of conditions AB ? You could check A alone and both in that [then] and [else] check B. (I hope that makes sense...)

Oh, it does make sense . But my code was just a quick example to test and to illustrate the difference in readability, it doesn't refer to any concrete situation.

I would write extract_children thus —

Sorry to disappoint you - but I decided to remove that function. As it turns out, using a if/elseif/else cycle resulted faster.

Secondly, you twice return after a helper.wml_error call. helper.wml_error immediately aborts down the call stack to the first protected (engine or pcall) function call, so a subsequent return is pointless — it will never be executed, except perhaps if the engine or its state is corrupt.

That's what happens when I attempt to code without my Lua reference handy...

What is the use of allowing multiple [then] or [else] subtags in [if], and why the inconsistency with [elseif]?

That's not an inconsistency, that's an outright bug. The idea was to allow multiple [then] in both [if] and [elseif], and multiple [else] in [if]. I placed the return statement in the wrong place and noticed it immediatly after that I clicked the Submit button.

Fourthly and finally, you use, e.g., helper.child_range ( then_children, "then" ) — why filter for [then] subtags, when then_children can contain nothing else (unless it is corrupt)?

Because my objective was to make the tag working ASAP, and only then try to fine-tune it. After testing it, I can confirm that it works well even with ipairs, so no need to use child_range.
So, I addressed all your issued and here there is a new version. I also managed to do the initial split in a single pass:

Code: Select all

local function if_handler_bis( cfg )
	-- [elseif] tags will contain conditions as well, so we need to remove them
	-- otherwise they'll interfere with [if]
	local if_cfg = {}
	local then_children = {}
	local elseif_children = {}
	local else_children = {}

	for index, value in ipairs( helper.shallow_literal( cfg ) ) do -- shallow_literal is faster than literal
		-- append only the WML table to the tables above: it results faster
		if value[1] == "then" then -- insulate [then]
			table.insert( then_children, value[2] )
		elseif value[1] == "elseif" then -- insulate [elseif]
			table.insert( elseif_children, value[2] )
		elseif value[1] == "else" then -- insulate [else]
			table.insert( else_children, value[2] )
		else -- send everything else to [if] for evalutation
			table.insert( if_cfg, value )
		end
	end

	if #then_children < 1 then -- we don't have even a single [then] ?
		helper.wml_error "[if] missing required [then] tag"
	end

	if wesnoth.eval_conditional( if_cfg ) then -- we'll evalutate an [if] tag that contains only the conditions
		for index, then_child in ipairs( then_children ) do
			handle_event_commands( then_child )
		end
		return -- stop after executing [then] tags
	else
		for index, elseif_child in ipairs( elseif_children ) do
			-- there's no point in executing [elseif] without [then]
			if not helper.get_child( elseif_child, "then" ) then
				helper.wml_error "[elseif] missing required [then] tag"
			end
			if wesnoth.eval_conditional( elseif_child ) then -- we'll evalutate the [elseif] tags one by one
				for then_tag in helper.child_range( elseif_child, "then" ) do
					handle_event_commands( then_tag )
				end
				return -- stop on first matched condition
			end
		end
		-- no matched condition, try the [else] tags
		--for else_child in helper.child_range ( else_children, "else" ) do
		for index, else_child in ipairs( else_children ) do
			handle_event_commands( else_child )
		end
	end
end

wml_actions["if"] = if_handler_bis

After testing it, this new [if] results faster than my former version, but still slower than the core one. The benchmark results were:
6897 6904 6882.
Another issue that I found is that [elseif] doesn't work inside [story] and [part]. The reason is quickly explained: in controller.cpp and part.cpp we can find the following block that needs to be fixed, to avoid inconsistency:

Code: Select all

		// [if]
		else if(key == "if") {
			const std::string branch_label =
				game_events::conditional_passed(node) ?
				"then" : "else";
			if(node.has_child(branch_label)) {
				const vconfig branch = node.child(branch_label);
				resolve_wml(branch);
			}
		}

That, however, will require me to understand how vconfig works and where it is defined - after that, it won't be much hard to add four std::vector to that block, iterate the vconfig and then handle them exactly as I did with Lua. At least, I hope so .

[Elvish_Hunter]

OK, time to keep you updated on the second leg of my quest.
So, I expected to use std::vector, vconfig, config and some other things while coding the C++ part. Not only attempting to do so gifted me with an endless supply of errors , but the scarce comments in the source made my task much harder.
However, I managed to cobble together something that seems to work well in C++. I hope that my comments will be enough as explanation. No benchmarks here, as UMC authors are much more likely to use repeated [if] checks inside events rather than story screens. That code will be added in both controller.cpp and part.cpp.

Code: Select all

		else if(key == "if") {
			// check if the [if] tag has a [then] child;
			// if not, raise a WML error and exit to make the mistake as much evident as possible
			// if we try to execute a non-existing [then], we get a segfault
			if (!node.has_child("then")) {
				lg::wml_error << "[if] missing required [then] tag\n";
				return;
			}
			else {
				// condition passed, execute [then]
				if (game_events::conditional_passed(node)) {
					resolve_wml(node.child("then"));
				}
				// condition not passed, check [elseif] and [else]
				else {
					// get all [elseif] children and set a flag
					vconfig::child_list elseif_children = node.get_children("elseif");
					bool elseif_flag = false;
					// for each [elseif]: test if it has a [then] child
					// if not, raise a WML error and exit
					// if yes, check condition; if matches, execute [then] and raise flag
					for (vconfig::child_list::const_iterator elseif = elseif_children.begin(); elseif != elseif_children.end(); ++elseif) {
						if (!elseif->has_child("then")) {
							lg::wml_error << "[elseif] missing required [then] tag\n";
							return;
						}
						else {
							if (game_events::conditional_passed(*elseif)) {
								resolve_wml(elseif->child("then"));
								elseif_flag = true;
								break;
							}
						}
					}
					// if we have an [else] tag and no [elseif] was successful (flag not raised), execute it
					if (node.has_child("else") && !elseif_flag) {
						resolve_wml(node.child("else"));
					}
				}
			}
		}

In that case, it wasn't even necessary to split the vconfig, as I originally thought .
And now, some Lua: I backported these C++ changes to my Lua code:

Code: Select all

local function if_handler_bis( cfg )
	-- raise error if [then] is missing
	if not helper.get_child( cfg, "then" ) then
		helper.wml_error "[if] missing required [then] tag"
	end

	if wesnoth.eval_conditional( cfg ) then -- evalutate [if] tag
		for then_child in helper.child_range ( cfg, "then" ) do
			handle_event_commands( then_child )
		end
		return -- stop after executing [then] tags
	else
		for elseif_child in helper.child_range ( cfg, "elseif" ) do
			-- there's no point in executing [elseif] without [then]
			if not helper.get_child( elseif_child, "then" ) then
				helper.wml_error "[elseif] missing required [then] tag"
			end
			if wesnoth.eval_conditional( elseif_child ) then -- we'll evalutate the [elseif] tags one by one
				for then_tag in helper.child_range( elseif_child, "then" ) do
					handle_event_commands( then_tag )
				end
				return -- stop on first matched condition
			end
		end
		-- no matched condition, try the [else] tags
		for else_child in helper.child_range ( cfg, "else" ) do
			handle_event_commands( else_child )
		end
	end
end

wml_actions["if"] = if_handler_bis

As you can see, now the code is much simpler, but a bit slower. These were the benchmark results:
7136 7126 7192
It's interesting to note that, for some reason, wesnoth.eval_conditional now works perfectly. When I tested it several months ago, this line:

Code: Select all

if wesnoth.eval_conditional( cfg ) then -- evalutate [if] tag

apparently catched and evalutated not only the conditional tags inside [if], but also those inside [elseif]. Almost certainly it was a bug with the code that I wrote back then...
That said, the Lua and C++ parts seems to be taken care of (unless more testing shows up other issues). Now I'll try and write the wmllint patch.
By the way: split and moved to Coder's Corner, because the patch isn't limited to Lua any more.

[Elvish_Hunter]

Third leg of my quest. This time, I'll talk about the wmllint patch. Here there is what I prepared so far:

Code: Select all

    # Sanity check nested [else] [if]
    # Starting from 1.13-svn, support for [elseif] inside [if] will be added
    # this allows writing conditional WML with better indentation
    # older code may or may not need conversion; this is left to the UMC author
    # in case that the nested [if] shouldn't be converted, use the magic comment
    # wmllint: elseifcheck off/on
    elseifcheck = True
    # check only [story], [part] and [event]; skip AnimationWML
    # because it has a different syntax
    in_story = False
    in_part = False
    in_event = False
    in_object = False # needed to avoid checking [object] [then]
    if_stack = 0
    else_stack = 0
    for i in xrange(len(lines)):
        if "wmllint: elseifcheck off" in lines[i]:
            elseifcheck = False
        elif "wmllint: elseifcheck on" in lines[i]:
            elseifcheck = True
        elif "[story]" in lines[i]:
            in_story = True
        elif "[/story]" in lines[i]:
            in_story = False
        elif "[part]" in lines[i]:
            in_part = True
        elif "[/part]" in lines[i]:
            in_part = False
        elif "[event]" in lines[i]:
            in_event = True
        elif "[/event]" in lines[i]:
            in_event = False
        elif "[object]" in lines[i]:
            in_object = True
        elif "[/object]" in lines[i]:
            in_object = False
        elif "[if]" in lines[i]:
            if_stack += 1
        elif "[/if]" in lines[i]:
            if_stack -= 1
        elif "[else]" in lines[i]:
            else_stack += 1
        elif "[/else]" in lines[i]:
            else_stack -= 1
        #if elseifcheck and (in_story or in_part or in_event) and in_else and not in_object:
        if elseifcheck and (in_story or in_part or in_event) and if_stack>0 and else_stack>0 and not in_object:
            if "[if]" in lines[i]:
                print '"%s", line %d: nested [else] [if] may need conversion to [elseif]' % (filename, i+1)

As you can see, this patch checks if we're inside a [story], [part] or [event] tag, that are the only places where [if] is allowed (besides AnimationWML, that has nothing to do with this topic).
If we're inside one of these tags, it checks if at least one [if] and one [else] are opened - hence the stack counters (by the way, I should really add another counter due to nested [event] tags). If yes, when another [if] is found, a message is printed. Nothing happens if we're inside an [object], because it uses both [then] and [else] for other purposes.
However, I'm not yet fully convinced by this wmllint patch. Does anyone see where it may be improved?

[zookeeper]

Well surely nothing "may need conversion" since the old if-else-if nesting will still work. I'm a bit suspicious whether such a check is of more use than noise in the first place; if you use [if]s a lot, then you're going to get a lot of those messages and converting a lot of old code to use [elseif] is probably not worth the gains in most cases anyway. It's a nice syntactic shortcut, but the wmllint notice is basically just saying "there's nothing wrong with your code but if you feel like it you can save a few lines here if you use this alternative syntax" so it seems a bit unnecessary.

Just my two cents, I'm not saying you shouldn't do it.

[Anonymissimus]

Zookeeper is being a nice guy so I'm the bad one for him. I say, "don't commit this" as wmllint should only complain about things that are or will actually be no longer supported or are bad in some way.
You should however provide the patch for anyone who wants to use it as a conversion help.

[Elvish_Hunter]

the wmllint notice is basically just saying "there's nothing wrong with your code but if you feel like it you can save a few lines here if you use this alternative syntax" so it seems a bit unnecessary.

Zookeeper is being a nice guy so I'm the bad one for him. I say, "don't commit this"

I see your points. I won't commit the wmllint patch, then.

You should however provide the patch for anyone who wants to use it as a conversion help.

That's an option; however, several users won't have any idea on how to patch their wmllint, or may accidentally break their own copy, or have to re-apply the patch every time that we release a new version of Wesnoth. It doesn't sound practical.
What I was thinking, instead, is that maybe I can create a small Python script that checks the .cfg files recycling the code that I already wrote, create a simple GUI with Tkinter/ttk (although now I can use QT/PySide as well, using it for such a small project seems like killing a spider with a flamethrower... ) and then package the program with Py2exe or cx-Freeze (for use on Windows). It's better?

[Iris]

Why not just add a new switch to wmllint to enable that and other future syntax recommendations on request?

[Elvish_Hunter]

Why not just add a new switch to wmllint to enable that and other future syntax recommendations on request?

That's a possible option - maybe using a --strict switch? However, I coded the GUI for the script:

There are still a few things that need to be worked out - like perhaps using the threading library (but I haven't yet found a simple enough tutorial about multithreading...) - after that, I'll post the source code.

[Iris]

However, I coded the GUI for the script

This is pretty much the equivalent of killing a fly with an atomic bomb.

*runs away from the thread*

[Elvish_Hunter]

This is pretty much the equivalent of killing a fly with an atomic bomb.

*runs away from the thread*

OK then. It wasn't much of a work for me, mainly because I had a lot of Tkinter code lying around that I simply needed to adapt (the whole thing required less than an hour). At least I have the whole feature freeze period to make up a good solution!

Anyway, big post this time! Sorry for the delay, but the holidays simply made impossible for me to work on this patch, and even to check the forum... Beside this, yesterday I managed to expand my improvements.
The next victims are [while] and [switch]/[case]. After my [elseif] modification, it became evident that the if_while_handler function in wml-tags.lua became unuseful. Its purpose was to have a single function handling both [if] and [while]; so, I decided to rework it only as while handler.

Code: Select all

local function while_handler_bis( cfg )
	-- check if the [do] sub-tag is missing, and raise error if so
	if not helper.get_child( cfg, "do" ) then
		helper.wml_error "[while] missing required [do] tag"
	end
	-- we have at least one [do], execute them up to 65536 times
	for i = 1, 65536 do
		if wesnoth.eval_conditional( cfg ) then
			for do_child in helper.child_range( cfg, "do" ) do
				handle_event_commands( do_child )
			end
		else return end
	end
end

wml_actions["while"] = while_handler_bis

No big changes here; however, we can see that if [do] is missing, an explicit error is raised (no more failing silently).
More interesting was altering [switch] [case]. This required me to work on both C++ and Lua. Lua code:

Code: Select all

function wml_actions.switch(cfg)
	-- check if variable= is missing
	if not cfg.variable then
		helper.wml_error "[switch] missing required variable= attribute"
	end 
	local variable = wesnoth.get_variable(cfg.variable)
	local found = false

	-- check if the [case] sub-tag is missing, and raise error if so
	if not helper.get_child( cfg, "case" ) then
		helper.wml_error "[switch] missing required [case] tag"
	end

	-- Execute all the [case]s where the value matches.
	for case_child in helper.child_range(cfg, "case") do
		-- warn if value= isn't present; it may be false, so check only for nil
		if case_child.value == nil then
			helper.wml_error "[case] missing required value= attribute"
		end
		local match = false
		for w in string.gmatch(case_child.value, "[^%s,][^,]*") do
			if w == tostring(variable) then match = true ; break end
		end
		if match then
			handle_event_commands(case_child)
			found = true
		end
	end
	-- Otherwise execute [else] statements.
	if not found then
		for else_child in helper.child_range(cfg, "else") do
			handle_event_commands(else_child)
		end
	end
end

And C++ code:

Code: Select all

		// [switch]
		else if(key == "switch") {
			// raise a WML error and exit if variable= is missing
			if (!node.has_attribute("variable")) {
				lg::wml_error << "[switch] missing required variable= attribute\n";
				return;
			}
			const std::string var_name = node["variable"];
			const std::string var_actual_value = resources::gamedata->get_variable_const(var_name);
			bool case_not_found = true;

			// check if the [switch] tag has a [case] child;
			// if not, raise a WML error and exit to make the mistake as much evident as possible
			if (!node.has_child("case")) {
				lg::wml_error << "[switch] missing required [case] tag\n";
				return;
			}

			for(vconfig::all_children_iterator j = node.ordered_begin(); j != node.ordered_end(); ++j) {
				if(j->first != "case") continue;
				
				// raise a WML error and exit if value= is missing
				if (!(j->second).has_attribute("value")) {
					lg::wml_error << "[case] missing required value= attribute\n";
					return;
				}

				// Enter all matching cases.
				const std::string var_expected_value = (j->second)["value"];
			    if(var_actual_value == var_expected_value) {
					case_not_found = false;
					resolve_wml(j->second);
			    }
			}

			if(case_not_found) {
				for(vconfig::all_children_iterator j = node.ordered_begin(); j != node.ordered_end(); ++j) {
					if(j->first != "else") continue;

					// Enter all elses.
					resolve_wml(j->second);
				}
			}
		}

As you can see, I enabled [switch] to check if variable= and at least one [case] are present; if not, an error is raised. In the same manner, we'll see a nice WML error if [case] doesn't have its value= key. I also changed some variable names in Lua, to make them more clear.
I know, these changes aren't directly related to [elseif], but I placed them in this topic because they somewhat derive from it: [while] because I had a if_while_handler floating around that needed to find a new purpose in life , and both [while] and [switch] to have a single, uniform behaviour with [if]. After all, it was quite absurd to have an error thrown if [then] was missing but not if [do] or [case] were.
Anyway, I suspect that some UMC authors will hate me while porting their code to the 1.13 series: it's true that I made the code much stricter, but the silver lining here is that campaign coders will be able to potentially track and fix some possible obscure bugs in their code - and here I speak for direct experience, having personally missed [then] once or twice...
Finally, apparently both the C++ and Lua legs of this quest are complete, so I'll post here a patch for testing as soon as I can.

[Elvish_Hunter]

Double post, since edits aren't notified in any way...
As promised, here there is the patch for 1.11.7+svn that adds all the Lua and C++ modifications that I discussed above. Feel free to download and test it.

[Turuk]

Double post, since edits aren't notified in any way...

As the last poster in a thread, you can always copy the content of your last post, delete it, put it into a new post and add the edits. This effectively lets people know there is an unread post again.

Not that there's any issue with double posting in these circumstances, but just a tip.