search

ThePhrozenKeep / D2MOO

Reimplementation of the Diablo2 game coupled with patching facilities for modders.
MIT License
92 stars 28 forks source link

Error in control variables in sub_6FCF77E0 (SkillAss.cpp) #175

Open Araksson opened 4 hours ago

Araksson commented 4 hours ago

In the file SkillAss.cpp, When doing iterations by progressive skills, there is a second loop that uses "i" again as a control variable, it should be "j"

First control var https://github.com/ThePhrozenKeep/D2MOO/blob/e4306096435ec2af96ac36df07c1001f77698a88/source/D2Game/src/SKILLS/SkillAss.cpp#L1041

Second control var https://github.com/ThePhrozenKeep/D2MOO/blob/e4306096435ec2af96ac36df07c1001f77698a88/source/D2Game/src/SKILLS/SkillAss.cpp#L1076

Necrolis commented 4 hours ago

This is technically a poor choice of naming, although not a bug due to C++ variable name shadowing rules (so the loops function correctly), see this short example: https://godbolt.org/z/j3PPjdEaf

Araksson commented 3 hours ago

In several places in D2Moo there are those errors of double nested 'for' with double control variable of the same name (I already reported them a while ago). In my case the compiler gives me a warning (because I have the option to show all warnings enabled) and when I run it it is actually "adding" invalid data to i within that loop and clearly the largest loop always starts again from 0-1-2 (depending on the value of the last progressive processed). And clearly it is an error because that variable is technically initialized twice.

Necrolis commented 2 hours ago

And clearly it is an error because that variable is technically initialized twice.

its not if its declared twice at different scopes, please see the C++ block scoping rules here. Which is why you'll see the following when /Wall is enabled:

D2MOO\source\D2Game\src\SKILLS\SkillAss.cpp(1079,42): warning C4456: declaration of 'i' hides previous local declaration

If you are experiencing bugs its likely as a result of the second declaration being missing.

I already reported them a while ago

If they were not created as tickets, please do list all the refs here, since they should be changed, as variable name shadowing is a "bad code smell".

@Lectem Alternately this ticket can be changed to a broad fix-all for warning C4456 outputs under /Wall (which IMO makes more sense).

Araksson commented 1 hour ago

I reported the other errors similar to this one to Lecten quite a while ago, I think they fixed them. I don't remember exactly where those errors are now to check them one by one, but they were in StatList, DRLG, ITEMS and PATH. But as I said, those codes were rewritten several times in D2Moo and the latest versions work well, so they must be corrected.