Skip to content

Conversation

Diapolo
Copy link

@Diapolo Diapolo commented May 31, 2015

No description provided.

@laanwj
Copy link
Member

laanwj commented Jun 1, 2015

There are still a lot of these cases where a const reference can be used for strings throughout the source code. Instead of many little pulls I'd prefer (for post-0.11) a pull where they're all addressed at once.

A quick git grep string -- \*.h|grep -v '&' shows at least:

  • FormatParagraph
  • CAlert::AppliesTo
  • ParseScript()
  • GetWarnings()
  • ParseNetwork()
  • CRPCTable::operator[](std::string name)
  • CRPCTable::help(std::string name)
  • HelpExampleCli
  • HelpExampleRpc
  • runCommand()
  • CWallet::GetAccountAddresses
  • CWallet::CWallet(std::string strWalletFileIn)
  • CValidationState::Error
  • CNode(SOCKET hSocketIn, CAddress addrIn, std::string addrNameIn = "", bool fInboundIn=false)

@Diapolo
Copy link
Author

Diapolo commented Jun 1, 2015

I'm going to update this :).

@Diapolo
Copy link
Author

Diapolo commented Jun 1, 2015

ParseNetwork() isn't suitable, because we use boost::to_lower(net); inside.

@Diapolo Diapolo changed the title [net] make AddOneShot take a const reference use const references where appropriate Jun 1, 2015
@Diapolo
Copy link
Author

Diapolo commented Jun 1, 2015

@laanwj I catched all your results and also changed a few other occurances in BOOST_FOREACH loops and in the CNode constructor (unrelated to std::string). Mind reviewing it?

@@ -1906,7 +1906,7 @@ bool CAddrDB::Read(CAddrMan& addr)
unsigned int ReceiveFloodSize() { return 1000*GetArg("-maxreceivebuffer", 5*1000); }
unsigned int SendBufferSize() { return 1000*GetArg("-maxsendbuffer", 1*1000); }

CNode::CNode(SOCKET hSocketIn, CAddress addrIn, std::string addrNameIn, bool fInboundIn) :
CNode::CNode(const SOCKET& hSocketIn, const CAddress& addrIn, const std::string& addrNameIn, bool fInboundIn) :
Copy link
Member

Choose a reason for hiding this comment

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

No need to do this for SOCKET, which is just an integer handle.

Copy link
Author

Choose a reason for hiding this comment

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

You are right, reverted for hSocketIn :), thanks.

@Diapolo
Copy link
Author

Diapolo commented Jun 1, 2015

@laanwj I'm going to attach a second squashme pull, which covers BOOST_FOREACH and updated to constant refs if appropriate, if you want me to do so?

@laanwj
Copy link
Member

laanwj commented Jun 1, 2015

utACK

@laanwj
Copy link
Member

laanwj commented Jun 1, 2015

I'm going to attach a second squashme pull, which covers BOOST_FOREACH and updated to constant refs if appropriate, if you want me to do so?

Yes, why not

@Diapolo
Copy link
Author

Diapolo commented Jun 1, 2015

@laanwj See 2nd commit.

@laanwj
Copy link
Member

laanwj commented Jun 2, 2015

utACK^2 . WIll merge this after #6121

@Diapolo
Copy link
Author

Diapolo commented Jun 2, 2015

That's fine, could be I forgot a few cases in the GUI code, but that can be a seperate pull.

@fanquake
Copy link
Member

fanquake commented Jun 3, 2015

utACK

@laanwj
Copy link
Member

laanwj commented Jun 4, 2015

Will merge after rebase.
@Diapolo can you please include getValStr() https://github.com/bitcoin/bitcoin/blob/master/src/univalue/univalue.h#L64 to return a const reference?

@Diapolo
Copy link
Author

Diapolo commented Jun 4, 2015

@laanwj Fixed 2 merge conflicts, one in rpcnet.cpp and the other in utilstrencodings.cpp and added getValStr(). Should be ready.

@laanwj laanwj merged commit a9ac95c into bitcoin:master Jun 5, 2015
laanwj added a commit that referenced this pull request Jun 5, 2015
a9ac95c use const references where appropriate (Philip Kaufmann)
@Diapolo Diapolo deleted the network branch June 5, 2015 07:10
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants