Skip to content
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

Remove duplicate mapWallet lookups #12276

Merged
merged 1 commit into from Jan 30, 2018
Merged

Conversation

promag
Copy link
Member

@promag promag commented Jan 26, 2018

No description provided.

@promag promag changed the title [wallet] Remove duplicate mapWallet lookups Remove duplicate mapWallet lookups Jan 26, 2018
@bitcoin bitcoin deleted a comment Jan 27, 2018

mapWallet[hash] = wtxIn;
CWalletTx& wtx = mapWallet[hash];
CWalletTx& wtx = mapWallet[hash] = wtxIn;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this isn't correct: C syntax A = B = C = D = X sets A,B,C,D to X.. So it assigns wtx to a reference to wtxIn, not to one to mapWallet[hash] as before.

Copy link
Member Author

@promag promag Jan 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laanwj the following:

#include <map>
#include <string>
#include <iostream>
using namespace std;
int main() {
  map<int,string> m;
  string s = "hello";
  string& r = m[1] = s;
  r.append(" world");
  cout << s << endl;
  cout << r << endl;
  cout << m[1] << endl;
  return 0;
}

gives:

hello
hello world
hello world

So I think it's correct. I can make it more explicit and still avoid the 2nd lookup.

See http://en.cppreference.com/w/cpp/container/map/operator_at, returns the reference, and the right = expression evaluates to that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are correct. Assignment happens from right to left. However I am not really a fan of the multiple assignment per line in this case since we are mixing reverence and value type assigmnents. It's a personal thing I guess but I usually like to separate such logic in multi-line as it helps with debugging etc. The compiler will optimize it anyway :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

@laanwj laanwj Jan 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know, thanks, didn't realize that, but yes it's better to avoid it to avoid confusion. The new formulation is more explicit.

@bitcoin bitcoin deleted a comment Jan 28, 2018
@bitcoin bitcoin deleted a comment Jan 28, 2018
@promag
Copy link
Member Author

promag commented Jan 29, 2018

Have to measure if it's of interest, but I suspect this improves the loading of big wallets a bit.

@jonasschnelli
Copy link
Contributor

utACK 039425c

@donaloconnor
Copy link
Contributor

utACK 039425c

@laanwj
Copy link
Member

laanwj commented Jan 30, 2018

utACK 039425c

@laanwj laanwj merged commit 039425c into bitcoin:master Jan 30, 2018
laanwj added a commit that referenced this pull request Jan 30, 2018
039425c [wallet] Remove duplicate mapWallet lookups (João Barbosa)

Pull request description:

Tree-SHA512: 8075925d2adb64737c691e988d74a37bc326711aaee2c37327361679c051f219fa500e14cbcdb6a169352bcdbab160e11df4276b2657e19e12908ee2d4444d30
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 17, 2020
039425c [wallet] Remove duplicate mapWallet lookups (João Barbosa)

Pull request description:

Tree-SHA512: 8075925d2adb64737c691e988d74a37bc326711aaee2c37327361679c051f219fa500e14cbcdb6a169352bcdbab160e11df4276b2657e19e12908ee2d4444d30
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 22, 2020
039425c [wallet] Remove duplicate mapWallet lookups (João Barbosa)

Pull request description:

Tree-SHA512: 8075925d2adb64737c691e988d74a37bc326711aaee2c37327361679c051f219fa500e14cbcdb6a169352bcdbab160e11df4276b2657e19e12908ee2d4444d30
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 22, 2020
039425c [wallet] Remove duplicate mapWallet lookups (João Barbosa)

Pull request description:

Tree-SHA512: 8075925d2adb64737c691e988d74a37bc326711aaee2c37327361679c051f219fa500e14cbcdb6a169352bcdbab160e11df4276b2657e19e12908ee2d4444d30
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2020
039425c [wallet] Remove duplicate mapWallet lookups (João Barbosa)

Pull request description:

Tree-SHA512: 8075925d2adb64737c691e988d74a37bc326711aaee2c37327361679c051f219fa500e14cbcdb6a169352bcdbab160e11df4276b2657e19e12908ee2d4444d30
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2020
039425c [wallet] Remove duplicate mapWallet lookups (João Barbosa)

Pull request description:

Tree-SHA512: 8075925d2adb64737c691e988d74a37bc326711aaee2c37327361679c051f219fa500e14cbcdb6a169352bcdbab160e11df4276b2657e19e12908ee2d4444d30
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2020
039425c [wallet] Remove duplicate mapWallet lookups (João Barbosa)

Pull request description:

Tree-SHA512: 8075925d2adb64737c691e988d74a37bc326711aaee2c37327361679c051f219fa500e14cbcdb6a169352bcdbab160e11df4276b2657e19e12908ee2d4444d30
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 31, 2020
039425c [wallet] Remove duplicate mapWallet lookups (João Barbosa)

Pull request description:

Tree-SHA512: 8075925d2adb64737c691e988d74a37bc326711aaee2c37327361679c051f219fa500e14cbcdb6a169352bcdbab160e11df4276b2657e19e12908ee2d4444d30
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants