-
Notifications
You must be signed in to change notification settings - Fork 1.2k
replace map count/insert w/emplace in instantx.cpp #2165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This feels like an optimization, something about:
```
if map.count(key)
return false
else
insert(pair)
return true
```
... just feels icky. Plus, `vote.GetMasternodeOutpoint()` is only called
once. The previous version might be slightly more readable, however.
src/instantx.cpp
Outdated
| return false; | ||
| mapMasternodeVotes.insert(std::make_pair(vote.GetMasternodeOutpoint(), vote)); | ||
| return true; | ||
| return mapMasternodeVotes.emplace(std::make_pair(vote.GetMasternodeOutpoint(), vote)).second; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does the .second do in this instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
emplace is a method defined on std::map, so I will point to the documentation there (in case you're interested):
Return value
Returns a pair consisting of an iterator to the inserted element, or the already-existing element if no insertion happened, and a bool denoting whether the insertion took place. True for Insertion, False for No Insertion.
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, that makes more sense. I guess I missed the first part of the return doc when I read it a few minutes ago. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for make_pair here with emplace.
UdjinM6
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, can be simplified a bit more though, see inline comment ;)
Also, maybe apply a similar fix to AcceptLockRequest/RejectLockRequest while at it?
src/instantx.cpp
Outdated
| return false; | ||
| mapMasternodeVotes.insert(std::make_pair(vote.GetMasternodeOutpoint(), vote)); | ||
| return true; | ||
| return mapMasternodeVotes.emplace(std::make_pair(vote.GetMasternodeOutpoint(), vote)).second; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for make_pair here with emplace.
|
You're right, good catch.
These functions just call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, ignore that part :) That should probably be in some another, more general PR.
utACK
|
@UdjinM6 I fully agree: nmarley@03af44a ;-) Would like to get that larger PR in after iterator cleanup if possible, so easier to merge w/o conflicts. |
codablock
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
* replace map count/insert w/emplace in instantx.cpp
This feels like an optimization, something about:
```
if map.count(key)
return false
else
insert(pair)
return true
```
... just feels icky. Plus, `vote.GetMasternodeOutpoint()` is only called
once. The previous version might be slightly more readable, however.
* use std::pair template constructor shortcut
This feels like an optimization, something about:
... just feels icky. Plus,
vote.GetMasternodeOutpoint()is only calledonce. The previous version might be slightly more readable, however.