Trivial: tiny c++11 refactors

[rodentrabies]

Changes for #8350

[fanquake]

Can you please improve the commit message to be more explicit about what is actually being changed.

[rodentrabies]

Yep, sorry. Done.

[theuni]

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

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

[dcousens]

utACK 3debfbc

[theuni]

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.

[dcousens]

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;
}

[dcousens]

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

[theuni]

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.

[laanwj]

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.

[rodentrabies]

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 (comment):

@@ -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
.

[dcousens]

@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.

[sipa]

Concept ACK, but the erase trick needs a comment to explain the behaviour.

[rodentrabies]

Added an explanation for the map::erase() usage with an SO link, mentioned above.

[dcousens]

utACK c784086

[sipa]

utACK. Did you see 5e187e7#r71238406 ?

EDIT: nevermind, i was confused by github.

[rodentrabies]

@sipa note about using emplace()? Yes, the last commit in this PR does that.

[sipa]

utACK c784086

[laanwj]

utACK c784086