iterator cleanup in several places

[nmarley]

Please see individual commits. A few notes:

• this should be all Dash-specific code, so none of these should conflict w/Bitcoin PRs
• mostly uses auto and range-based for

[UdjinM6]

Looks good in general but it feels a bit awkward to see iterators-like names it/itOutpointLock/etc inside loop blocks. Let's clean this up pls e.g. s/itOutpointLock/outpointLock/ etc.

[nmarley]

You're probably right. What would you suggest I use for a std::map<int, CSporkMessage> entry though (from the 1st commit for example)? Just entry, or pair, or ... ?

Or are you only suggesting this for the longer it[A-Za-z]? names?

[UdjinM6]

For simple cases like that (literally two rows of code) pair would do it I guess. For mapMasternodes it's mnpair all over the place already, for mMnbRecoveryGoodReplies (which is a bit more code) could be mnbreplypair to give some context or smth like that.

[nmarley]

Ok, I believe I have addressed the naming issues. I have also cleaned up a few more places in the code where range-based for and auto can be used.

[UdjinM6]

You can't do this

for (auto& pair : someMap) {
    auto key = pair.first;
    someMap.erase(key);
}

It's going to segfault.

[nmarley]

@UdjinM6 Works fine in my test.

Code: https://gitlab.com/nmarley/cpptest/blob/master/range-for-modify.cpp

Output: https://gist.github.com/nmarley/c28de321bd15d1205900b1cf9d595d2e

[UdjinM6]

Code: https://gitlab.com/nmarley/cpptest/blob/master/range-for-modify.cpp

The page could not be found or you don't have permission to view it.

[nmarley]

@UdjinM6 Sorry, should be public now. And you're right, looks like this code was "working" because of some internal ordering of keys. Once I tried deleting element 13 instead, things got screwy.

I will go back and refactor (or revert) these. I was hoping to get rid of all the incrementing iterator else blocks this way. :/

[UdjinM6]

Well, if that was the goal, you could probably try smth like:

auto it = someMap.begin();
while (it != someMap.end()) {
    auto curr = it++;
    if (condition) {
        // do smth using curr
        someMap.erase(curr);
    }
}

EDIT:

or if you REEEALLY want to use for, you could probably even do smth like:

for (auto it = someMap.begin(); it != someMap.end(); ) {
    auto curr = it++;
    if (condition) {
        // do smth using curr
        someMap.erase(curr);
    }
}

:)

[UdjinM6]

The problem with the approach above is that this way you always create a copy, even if you don't need it, while with else ++it; you can avoid it (in some cases most of the time).

EDIT: however for small maps the overhead is probably negligible.

[nmarley]

Well, my idea was actually to invert the if / else condition such that I can if (thing) { it++ ; continue; } and remove the else block that way. But might do that in a separate refactor PR.

[nmarley]

I've reverted changes where I made the false assuption that a container being iterated over could be modified using range-based for, which isn't the case. Also rebased onto the latest develop and squashed all since I reverted most of the changes that I'd originally made.

[UdjinM6]

Pushed few suggestions to my repo, see individual commits:

• could use const refs in few more places 401be7b
• some names are still confusing a6084bb
• use prefix ++ e4a4e59

Otherwise looks good 👍

[nmarley]

Just had one question on the rationale for ++thing vs thing++, but looks good, thanks! 👍

edit: I fast-forwarded my branch tip to include your commits directly.

[UdjinM6]

ACK