helper.literal, .parsed etc semantics discussion
Moderator: Forum Moderators
-
- Inactive Developer
- Posts: 2461
- Joined: August 15th, 2008, 8:46 pm
- Location: Germany
helper.literal, .parsed etc semantics discussion
Ok, now it did happen to me that I accidentally "passed by reference" and thereby modified a passed wml table at the position where helper.parsed was called, although I wanted to modify only the table I got from helper.parsed. In case that userdata is passed to those functions, all of the __literal, __shallow_literal, __parsed, __shallow_parsed fields create a copy, making it impossible to accidentally modify the table at the calling position, right ? I think that in case that a table is passed it would be better to create a copy instead of returning the table itsself...
At least this is very tricky and should be mentioned in the wiki then, "advanced lua traps" or so...
Also, the [move_unit] tag, as it is now in wml-tags.lua, is already dangerous btw: I did set several attributes to nil, modifying a table at where the function is called if it has been called from lua by passing a wml table. I should instead have created a copy (but only in case that a table has been passed).
EDIT
Adding a helper.shallow_copy function makes at least sense; so that when a function wants to modify its parameter table which could be a table from some other lua code, it must create a copy with it, and that the helper.literal etc functions do not automatically create copies all of the time, even in case it's not needed.
At least this is very tricky and should be mentioned in the wiki then, "advanced lua traps" or so...
Also, the [move_unit] tag, as it is now in wml-tags.lua, is already dangerous btw: I did set several attributes to nil, modifying a table at where the function is called if it has been called from lua by passing a wml table. I should instead have created a copy (but only in case that a table has been passed).
EDIT
Adding a helper.shallow_copy function makes at least sense; so that when a function wants to modify its parameter table which could be a table from some other lua code, it must create a copy with it, and that the helper.literal etc functions do not automatically create copies all of the time, even in case it's not needed.
Last edited by Anonymissimus on October 22nd, 2010, 12:44 pm, edited 1 time in total.
projects (BfW 1.12):
A Simple Campaign: campaign draft for wml starters • Plan Your Advancements: mp mod
The Earth's Gut: sp campaign • Settlers of Wesnoth: mp scenario • Wesnoth Lua Pack: lua tags and utils
updated to 1.8 and handed over: A Gryphon's Tale: sp campaign
A Simple Campaign: campaign draft for wml starters • Plan Your Advancements: mp mod
The Earth's Gut: sp campaign • Settlers of Wesnoth: mp scenario • Wesnoth Lua Pack: lua tags and utils
updated to 1.8 and handed over: A Gryphon's Tale: sp campaign
Re: helper.literal, .parsed etc syntax discussion
(Note that your post doesn't have anything to do with syntax, but with semantic.)
There was a tradeoff between simplicity and performances when I designed these functions and I have changed my mind a few times about them, as can be seen in the svn logs. In the end, I decided for performances: since most usages of wml actions don't reuse the parameter for subsequent actions, there is no point in preserving it. The current implementation is such that there is no inefficiency when tags are called the following way:
Still, there are some cases where the same parameter will be reused over and over. Then it's up to the user to take care of preserving the parameter so that it is still there after invoking a tag. There are two ways of doing it. One is to call wesnoth.tovconfig to abstract the table away. The other is to call wesnoth.fire instead of wesnoth.wml_actions. (The two ways are in fact exactly equivalent, by definition of wesnoth.fire.) I thought I had written something to that intent in the wiki, but I can only find the part about variable substitution, not the one about parameter decay.
So [move_unit] doesn't need to be modified. And helper.shallow_copy is useful in general, but not because of the semantic of helper.literal.
There was a tradeoff between simplicity and performances when I designed these functions and I have changed my mind a few times about them, as can be seen in the svn logs. In the end, I decided for performances: since most usages of wml actions don't reuse the parameter for subsequent actions, there is no point in preserving it. The current implementation is such that there is no inefficiency when tags are called the following way:
Code: Select all
wml_actions.do_something { foo = 17, bar = _ "Baz" }
So [move_unit] doesn't need to be modified. And helper.shallow_copy is useful in general, but not because of the semantic of helper.literal.
-
- Inactive Developer
- Posts: 2461
- Joined: August 15th, 2008, 8:46 pm
- Location: Germany
Re: helper.literal, .parsed etc syntax discussion
OK, so you are suggesting that whenever some lua code calls wml_actions.something and reuses the passed table (if it's a table at all) afterwards, it should take care for that the called tag may modify the passed table ? (We could look whether it actually does, but once its code gets changed it may do and introduce a bug...)
EDIT
In some earlier post, you suggested that the called function should be reponsible for not modifying its parameters/creating a copy:
Similar issue can happen when passing a wml table to e.g. wml_actions.move_unit, and it can one make searching bugs for hours...
What about a deep copy instead, would that be less efficient ? Since the intention here is not to enforce variable substitution but only to make the parameter "const".One is to call wesnoth.tovconfig to abstract the table away.
EDIT
In some earlier post, you suggested that the called function should be reponsible for not modifying its parameters/creating a copy:
For anyone trying to understand the issue I've created a small example demonstrating it:silene wrote:Either a function documents that it modifies some of its arguments, or it should perform a shallow copy of all the tables (possibly nested ones) it modifies.
Code: Select all
local function modify_table(t)
t[1] = 2
end
local t = { 1, 1}
print(t[1]) -- "1"
modify_table(t)
print(t[1]) -- "2" !!
projects (BfW 1.12):
A Simple Campaign: campaign draft for wml starters • Plan Your Advancements: mp mod
The Earth's Gut: sp campaign • Settlers of Wesnoth: mp scenario • Wesnoth Lua Pack: lua tags and utils
updated to 1.8 and handed over: A Gryphon's Tale: sp campaign
A Simple Campaign: campaign draft for wml starters • Plan Your Advancements: mp mod
The Earth's Gut: sp campaign • Settlers of Wesnoth: mp scenario • Wesnoth Lua Pack: lua tags and utils
updated to 1.8 and handed over: A Gryphon's Tale: sp campaign
Re: helper.literal, .parsed etc syntax discussion
Yes. In other words, it should not call two tags in a row with the same tables.Anonymissimus wrote:OK, so you are suggesting that whenever some lua code calls wml_actions.something and reuses the passed table (if it's a table at all) afterwards, it should take care for that the called tag may modify the passed table ?
Efficiency will depend on what the innermost code expects. For pure Lua tags (e.g. never passing a table to C++), the deep copy is probably faster. But if the table ends up being used as a filter, then tovconfig is faster (since that's what the C++ code will do anyway, and only the first tovconfig call is costly).Anonymissimus wrote:What about a deep copy instead, would that be less efficient ? Since the intention here is not to enforce variable substitution but only to make the parameter "const".One is to call wesnoth.tovconfig to abstract the table away.
It should either be responsible or document it isn't. As I said, I thought I had documented that calling tags directly through wml_actions could cause the tables to decay.Anonymissimus wrote:In some earlier post, you suggested that the called function should be reponsible for not modifying its parameters/creating a copy:silene wrote:Either a function documents that it modifies some of its arguments, or it should perform a shallow copy of all the tables (possibly nested ones) it modifies.
I understand that the current situation is not ideal and I'm open to suggestions. My goal was to ensure that the straightforward way of invoking tags was also the efficient one. Perhaps an improvement would be to document that only the toplevel fields might be modified. Then a shallow copy (or creation) would be sufficient to ensure that tables have to be reused for several tag invocations. Another possibility would be to introduce a __protect field: if there is one in a table, the helper functions would actually perform the shallow copy.
-
- Inactive Developer
- Posts: 2461
- Joined: August 15th, 2008, 8:46 pm
- Location: Germany
Re: helper.literal, .parsed etc semantics discussion
Imo the case that wml_actions.something is called with a freshly constructed table which is not reused afterwards is much more common, like so
Often there are variable values assigned, but they are mostly primitive types (like here; numbers, strings, nil and boolean) - these are non-pointer types and do value-assignment when used with "=" so it's uncritical. For the variable substitution part we already decided that the caller should be reponsible for deciding whether there may be variables to substitute...
Would this be an appropriate deep copy function ?
It runs into stack overflow for a table whose entry points to itsself however:
local t = { 1 }
t[2] = t
(Substitute the call to helper.deep_copy with helper.shallow_copy to allow the table to be modified.)
Code: Select all
wml_actions.move_unit({ id = "executioner", to_x = udPrisoner.x, to_y = udPrisoner.y})
Would this be an appropriate deep copy function ?
It runs into stack overflow for a table whose entry points to itsself however:
local t = { 1 }
t[2] = t
(Substitute the call to helper.deep_copy with helper.shallow_copy to allow the table to be modified.)
Code: Select all
function helper.shallow_copy(t)
local u = {}
for k,v in pairs(t) do
u[k] = v
end
setmetatable(u, getmetatable(t))
return u
end
function helper.deep_copy(table)
local result = {}
for current_key, current_value in pairs(table) do
if type(current_value) ~= "table" then
result[current_key] = current_value
else
result[current_key] = helper.deep_copy(current_value)
end
end
setmetatable(result, getmetatable(table))
return result
end
local function modify_table(t)
t[2][2] = 2
end
local function some_fun() end
local t = { 1, { 1, some_fun} }
wesnoth.message(tostring(t[2][2]))
modify_table(helper.deep_copy(t))
wesnoth.message(tostring(t[2][2]))
projects (BfW 1.12):
A Simple Campaign: campaign draft for wml starters • Plan Your Advancements: mp mod
The Earth's Gut: sp campaign • Settlers of Wesnoth: mp scenario • Wesnoth Lua Pack: lua tags and utils
updated to 1.8 and handed over: A Gryphon's Tale: sp campaign
A Simple Campaign: campaign draft for wml starters • Plan Your Advancements: mp mod
The Earth's Gut: sp campaign • Settlers of Wesnoth: mp scenario • Wesnoth Lua Pack: lua tags and utils
updated to 1.8 and handed over: A Gryphon's Tale: sp campaign
Re: helper.literal, .parsed etc semantics discussion
This is fine. Note that the stack overflow (and more generally the loss of structure for cyclic objects) can be easily avoided. It would look like: (untested)Anonymissimus wrote:Would this be an appropriate deep copy function ?
It runs into stack overflow for a table whose entry points to itsself however:
Code: Select all
function helper.deep_copy(source)
local copied = {}
local function doit(src)
local dst = {}
for k,v in pairs(src) do
if type(v) ~= "table" then
dst[k] = v
else
dst[k] = copied[v] or doit(v)
end
end
copied[src] = dst
return setmetatable(dst, getmetatable(src))
end
if type(source) ~= "table" then
return source
else
return doit(source)
end
end
-
- Inactive Developer
- Posts: 2461
- Joined: August 15th, 2008, 8:46 pm
- Location: Germany
Re: helper.literal, .parsed etc semantics discussion
Your code doesn't work, that is, it doesn't prevent the stack overflow. Also I don't understand the semantics, do you want to do something like
"execute the recursion until the overflow (would) occur but at that point cease and use the previous result" ?
"execute the recursion until the overflow (would) occur but at that point cease and use the previous result" ?
projects (BfW 1.12):
A Simple Campaign: campaign draft for wml starters • Plan Your Advancements: mp mod
The Earth's Gut: sp campaign • Settlers of Wesnoth: mp scenario • Wesnoth Lua Pack: lua tags and utils
updated to 1.8 and handed over: A Gryphon's Tale: sp campaign
A Simple Campaign: campaign draft for wml starters • Plan Your Advancements: mp mod
The Earth's Gut: sp campaign • Settlers of Wesnoth: mp scenario • Wesnoth Lua Pack: lua tags and utils
updated to 1.8 and handed over: A Gryphon's Tale: sp campaign
Re: helper.literal, .parsed etc semantics discussion
The 15th line was too late. Moving it earlier in the function fixes it. (And this time I tested it.)Anonymissimus wrote:Your code doesn't work, that is, it doesn't prevent the stack overflow.
Code: Select all
function helper.deep_copy(source)
local copied = {}
local function doit(src)
local dst = {}
copied[src] = dst
for k,v in pairs(src) do
if type(v) ~= "table" then
dst[k] = v
else
dst[k] = copied[v] or doit(v)
end
end
return setmetatable(dst, getmetatable(src))
end
if type(source) ~= "table" then
return source
else
return doit(source)
end
end
Not at all. The code just preserves the original structure. For instance, if you pass itAnonymissimus wrote:Also I don't understand the semantics, do you want to do something like
"execute the recursion until the overflow (would) occur but at that point cease and use the previous result" ?
Code: Select all
local t = { 1 }
t[2] = t
local u = helper.deep_copy(t)
Code: Select all
assert(u ~= t and u[1] == t[1] and u[2] == u)
-
- Inactive Developer
- Posts: 2461
- Joined: August 15th, 2008, 8:46 pm
- Location: Germany
Re: helper.literal, .parsed etc semantics discussion
I think I got it.
copied[src] = dst remembers for every encountered table a pointer to its copy.
dst[k] = copied[v] or doit(v) checks whether any value encountered, which we know is a table at that point, points to one of these tables from above. If no, copied[v] is nil and doit(v) is called, if yes, the pointer to the copy of that table is used for assignment.
Neat, actually. So the cyclic recursion is not started at all. Also strange and confusing that indices can be a table.
copied[src] = dst remembers for every encountered table a pointer to its copy.
dst[k] = copied[v] or doit(v) checks whether any value encountered, which we know is a table at that point, points to one of these tables from above. If no, copied[v] is nil and doit(v) is called, if yes, the pointer to the copy of that table is used for assignment.
Neat, actually. So the cyclic recursion is not started at all. Also strange and confusing that indices can be a table.
projects (BfW 1.12):
A Simple Campaign: campaign draft for wml starters • Plan Your Advancements: mp mod
The Earth's Gut: sp campaign • Settlers of Wesnoth: mp scenario • Wesnoth Lua Pack: lua tags and utils
updated to 1.8 and handed over: A Gryphon's Tale: sp campaign
A Simple Campaign: campaign draft for wml starters • Plan Your Advancements: mp mod
The Earth's Gut: sp campaign • Settlers of Wesnoth: mp scenario • Wesnoth Lua Pack: lua tags and utils
updated to 1.8 and handed over: A Gryphon's Tale: sp campaign
Re: helper.literal, .parsed etc semantics discussion
In fact, anything can be an index; copied[v] would have worked whether v was a table or not. For instance, if the doit function knew how to copy some other types than just tables, the inner loop could be as simple asAnonymissimus wrote:copied[src] = dst remembers for every encountered table a pointer to its copy.
dst[k] = copied[v] or doit(v) checks whether any value encountered, which we know is a table at that point, points to one of these tables from above. If no, copied[v] is nil and doit(v) is called, if yes, the pointer to the copy of that table is used for assignment.
Neat, actually. So the cyclic recursion is not started at all. Also strange and confusing that indices can be a table.
Code: Select all
for k,v in pairs(src) do
dst[k] = copied[v] or doit(v)
end