Change [filter_weapon] to not depend on attack_type::weapon_specials #6501
Conversation
stevecotton
added
Enhancement
github-actions
bot
added
the
WFL
|
@newfrenchy83 sorry it's taking so long to review that PR, but it hasn't been forgotten. I'm expecting the CI build to fail on flatpak, which is waiting for a new version of an SDK to be uploaded to dockerhub. |
|
@stevecotton i commited 27429f5 for resolve that. |
|
Thanks, but my motivation for writing this was to get rid of the |
when this PR will be merged, i remove only_active argument in my PR |
src/formula/callable_objects.cpp
Outdated
| const auto& s = self_specials[i].cfg["id"]; | ||
| const auto& o = other_specials[i].cfg["id"]; | ||
| if(s != o) { | ||
| return s.str() < o.str() ? -1 : 1; |
There was a problem hiding this comment.
Any reason not to just return s.compare(o)?
There was a problem hiding this comment.
Will change to s.str().compare(o.str()), thanks. They're both config_attribute_value objects, so there isn't a direct compare(), but I forgot to look again for a compare() function after realising that I needed to convert them to strings.
There was a problem hiding this comment.
I realize this is a bit late now that it's merged, but you could also do:
auto result = s.str().compare(o.str());
if(result != 0) return result;That way you compare it only once instead of potentially twice.
There was a problem hiding this comment.
Good idea, I'll remember that next time. However, for the backport I think it's good to keep the code the same as this already-merged version.
|
This seems fine to me. |
This changes it to use the function that attack_type::get_value already uses, thus removing a call to attack_type::weapon_specials. The latter is being refactored by newfrenchy83, and questions about the effect of that refactor on this code delayed the review on his PR. This code is part of the object used for [filter_weapon]formula=, however the implementation of that filter creates exactly one attack_type_callable, so doesn't trigger this code. For testing I called it via a debugger.
stevecotton
force-pushed
the
wfl_compare_weapon_specials
branch
from
b38b7df
to
040bb02
Compare
Fixes the currently suspected root cause of wesnoth#5108, but requires further testing to confirm that it fixes that issue. Closes wesnoth#6501 (the question of how to test the mapgen filter).
This changes it to use the function that attack_type::get_value already uses,
thus removing a call to attack_type::weapon_specials. The latter is being
refactored by @newfrenchy83, and questions about the effect of that refactor on
this code delayed the review on his PR.
This code is part of the object used for [filter_weapon]formula=, however the
implementation of that filter creates exactly one attack_type_callable, so
doesn't trigger this code. For testing I called it via a debugger.