On Feb 15, 2012, at 1:19 PM, Kris De Gussem wrote:
Hi all
During another round of cppcheck, I stumbled upon some weird code in sp-conn-end.cpp. Can someone have a look at line 77 to 85?
This block of code does not seem right (why is for-loop variable i used again as for loop variable in a sub loop?):
for (Geom::CrossingSet::const_iterator i = cross.begin(); i != cross.end(); i++) { const Geom::Crossings& cr = *i;
for (Geom::Crossings::const_iterator i = cr.begin(); i !=cr.end(); i++) {
Yes, that's not good code. It probably should be split into different names, since that really isn't maintainable regardless of functionality. I think perhaps the scoping rules might be helping it do what the author intended, but it still needs cleanup up.
One main style point would be to avoid the single letter variable. For an iterator, we should probably go with "it" instead of just "i".
Also, on the inner loop we have an if statement that does not have any braces. Would be good to get some in there.
Ideally we'd get some simple test in place to see that there is either no change in behavior, or bad behavior pre-changed matched with good behavior post-change.
Another test would be to find a usage that ensures the code is hit, then do that while running under Valgrind. It would normally then catch any bad iterator use/confusion.