Update of patch #848 (project wesnoth):
Status: None => Invalid
Assigned to: None => alink
_______________________________________________________
Follow-up Comment #1:
Well, yes but no I don't think so. I agree, it's a good catch of a implicit
assumption. I explain:
All the min_* variables stay in a consistent state. They are all initialized
at the same time, and note that min_types is initialized as an empty vector.
Then only if the (constraint_size < min_size) triggers, they are all set
together.
So the main point is that the code in the loop on min_types is executed only
when min_types is not empty and this can happen only if we have found a
min_constraint (the one which matches all the types of min_types).
So the proper readability/robustness fix is to add a guard before the
min_types loop :
if (min_constraint != rule.constraints.end())
To indicate and check that we have found one. A bit late to touch this
sensitive code before the soon 1.3.11, so i will do later (except if you spot
an error in my explanation). Or maybe just add more comments (which are a bit
short compared to the code complexity).
Btw, sorry but i don't think that your version with
"--(rule.constraint.end())" is a good idea, for two reasons,
- I didn't check the code but indeed I think that valid WML can create a rule
with an empty rule.constraint and just a rule.location_constraints, or at
least we want to support that.
- if we set min_constraint to --(rule.constraint.end()) and that my previous
explanation is wrong (or if the code change), we risk to use an incorrect
min_constraint in rules. Which can cause ugly big glitches or, worse, really
hard to debug terrain rule error. It's better to crash ;p (for the WML
terrain designers).
Also sorry for this maybe not clear code, I have started to optimized it but
it's not finished (i have an old pending commit for caching these
terrain_matches, first test seems to give a ~20% gain in heavy cases, but i
need to benchmark and optimize it more, because i don't like adding code
complexity for not clear gain). I also decided to wait the feature freeze
before continue my optimization. The code is already shorter and faster now
(changes done in r20852 and r20853), so it's stopped to be an urgent
problem.
And thanks to spot details like that, this is tricky code and hard to notice
incorrect result, I could easily let slip such errors.
_______________________________________________________
Reply to this item at:
<http://gna.org/patch/?848>
_______________________________________________
Message sent via/by Gna!
http://gna.org/
_______________________________________________
Wesnoth-bugs mailing list
[email protected]
https://mail.gna.org/listinfo/wesnoth-bugs
