Trivial: tiny c++11 refactors #8353

May as well use emplace here while you're at it:

deadlineTimers.emplace(name, std::unique_ptr<RPCTimerBase>(timerInterface->NewTimer(func, nSeconds*1000)));

This doesn't look right. Since erase returns the next iterator, I'm not sure what good it would do here. It's also erasing without re-adding.

As-is, this appears to be modifying a random element.

Actually, it doens't erase anything AFAIK. Note, erase works on [first, last).

It is totally confusing though, but I assumed for that comment to exist, this had already been bikeshed as somehow sensible.

Example:

#include <iostream>
#include <map>

int main ()
{
    std::map<char,int> mymap;

    // insert some values
    mymap['a'] = 10;
    mymap['b'] = 20;
    mymap['c'] = 30;
    mymap['d'] = 40;
    mymap['e'] = 50;
    mymap['f'] = 60;

    std::cout << mymap.size() << std::endl; // 6
    const auto x = mymap.find('d');
    mymap.erase(x, x);
    std::cout << mymap.size() << std::endl; // 6

    // show all
    for (auto y : mymap)
        std::cout << y.second << '\n'; // 10, 20, 30, 40, 50, 60

    return 0;
}

ping @TheBlueMatt (this was after all, your TODO)

Huh, indeed. Neat trick! Thanks for explaining. Is the returned iterator guaranteed to be the same as the inputs? Everything I can find references the return relative to the last item erased, which obviously doesn't apply here.

Assuming it's all well defined, fine by me, though it's much less clear what's happening.

I strongly prefer non-tricky code to tricky code, even if documented, to be honest. It increases the scope of making mistakes (due to misunderstanding the code) in later maintenance. So unless this makes a clear performance difference in practical usage I prefer keeping it as-is.

I currently don't have access to my laptop, so will bench and fix it accordingly in a couple of days.

On Aug 3, 2016 7:25 PM, "Wladimir J. van der Laan" notifications@github.com wrote:

In src/limitedmap.h #8353 (review):

@@ -66,8 +66,7 @@ class limitedmap } void update(const_iterator itIn, const mapped_type& v) {

•    // TODO: When we switch to C++11, use map.erase(itIn, itIn) to get the non-const iterator.

•    iterator itTarget = map.find(itIn->first);

•    iterator itTarget = map.erase(itIn, itIn);

I strongly prefer non-tricky code to tricky code, even if documented, to be honest. It increases the scope of making mistakes (due to misunderstanding the code) in later maintenance. So unless this makes a clear performance difference in practical usage I prefer keeping it as-is.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/8353/files/3debfbca0d85d7c17f67c5a15df0938584445026#r73304022, or mute the thread https://github.com/notifications/unsubscribe-auth/AHSu6XP5fp8pV6uoGcv3MUlHEtEF-t1Nks5qcFmmgaJpZM4JOU7Z .

@laanwj this trick is explained here: https://stackoverflow.com/questions/765148/how-to-remove-constness-of-const-iterator, and appears to be pretty common.

The access is constant time versus the lookup, so it is an improvement (assuming this is actually a performance bottleneck anyhow). I think a SO link and explanation would be more than fine to appropriate its existence.