Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate helper.get_sides #4049

Open
spixi opened this issue Apr 23, 2019 · 5 comments

Comments

Projects
None yet
3 participants
@spixi
Copy link
Contributor

commented Apr 23, 2019

helper.lua contains the method get_sides(cfg), which is member of the wesnoth table. helper.get_sides(cfg) should be deprecated in favor of wesnoth.get_sides(cfg).

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

I think there was a genuine reason for it not to be deprecated, but I'm not quite sure. Does wesnoth.get_sides do the exact same thing (returning an iterator over matching sides)? Or does it return a list rather than an iterator?

That said, even if it should remain in use, it should at least be moved from the helper module to somewhere more appropriate.

@spixi

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

wesnoth.get_sides(cfg) returns a list, but ipairs works for lists and iterators, so an explicit function, which returns an iterator is not really useful. As far, I can see, helper.get_sides(cfg) is unused in mainline. I don't know about UMCs, however.

There is actually a third get_sides(cfg, key_name, filter_name) function in /data/lua/wml_utils.lua which works somewhat different, because some WML tags like [remove_shroud], [place_shroud], [scroll], [scroll_to], [scroll_to_unit] and [modify_ai] support a standalone side= and a regular standard side filter. So I would keep this one, but only for internal usage in the WML engine.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

On the contrary, I'd say a function that returns an iterator is useful at least in a general case, as it's more efficient than returning a list. That said, there's probably no discernible difference for short lists, which includes the sides list. I'm fine with marking helper.get_sides deprecated (probably at the bottom of the file with all the other deprecated things) in favour of using ipairs on wesnoth.get_sides.

The other get_sides you mention is basically for internal usage in the WML engine, yes (though it's placed in wml_utils so that others can use it in their custom WML tags if they so desire).

@spixi

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

The table wesnoth.sides is redundant according to

// This table is redundant to the return value of wesnoth.get_sides({}).
The iterator uses that buffer instead of determine the current values each time.

I guess, the length of the lists does not really matter, because appending values to a vector and looping them has O(N), the same run-time behavior like an iterator over a vector. There is also no copy of values involved, references are cheap.

I also saw that std::vector<int> get_sides_vector(const vconfig& cfg) actually supports a single inline side= and a standard side filter. The only purpose of wml_utils.get_sides seems to be providing support for WML tags with non-standard names for [filter_side] and side=.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

I guess, the length of the lists does not really matter, because appending values to a vector and looping them has O(N), the same run-time behavior like an iterator over a vector. There is also no copy of values involved, references are cheap.

It probably matters for large lists where you break early - you're only processing the first few elements. If it's a list, the entire list needs to be generated first, including the many elements you don't even need, but if it's an iterator, that's not an issue.

I guess there's some truth in the "references are cheap", but I'm not sure if that's relevant when you're generating the list, which is probably what wesnoth.get_sides() does. True it's a list of references, but it still needs to be generated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.