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

util: Remove [U](BEGIN|END) macros #15139

Merged
merged 3 commits into from Jan 10, 2019

Conversation

Projects
None yet
7 participants
@laanwj
Copy link
Member

laanwj commented Jan 10, 2019

Two cleanups in util/strencodings.h:

  • Remove [U](BEGIN|END) macros — The only use of these was in the Merkle tree code with uint256 which has its own begin and end methods which are better.
  • Make ToLower and ToUpper take a char — Unfortunately, std::string elements are (bare) chars. As these are the most likely type to be passed to these functions, make them use char instead of unsigned char. This avoids some casts.

laanwj added some commits Jan 10, 2019

Replace use of BEGIN and END macros on uint256
Replace use of `BEGIN` and `END` macros on uint256 with `begin()` and
`end()` methods in the Merkle tree code.
util: Make ToLower and ToUpper take a char
Unfortunately, `std::string` elements are (bare) chars. As these
are the most likely type to be passed to these functions, make them use
char instead of unsigned char. This avoids some casts.

@laanwj laanwj force-pushed the laanwj:2018_01_utilstrencodings branch to 332b3dd Jan 10, 2019

@Empact

This comment has been minimized.

Copy link
Member

Empact commented Jan 10, 2019

utACK 332b3dd - could squash the first two

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Jan 10, 2019

utACK 332b3dd

@promag

This comment has been minimized.

Copy link
Member

promag commented Jan 10, 2019

utACK 332b3dd.

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Jan 10, 2019

utACK 332b3dd

Would be nice to have #15134 merged for extra checking of these types of changes :-)

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jan 10, 2019

utACK

@MarcoFalke MarcoFalke merged commit 332b3dd into bitcoin:master Jan 10, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Jan 10, 2019

Merge #15139: util: Remove [U](BEGIN|END) macros
332b3dd util: Make ToLower and ToUpper take a char (Wladimir J. van der Laan)
edb5bb3 util: remove unused [U](BEGIN|END) macros (Wladimir J. van der Laan)
7fa238c Replace use of BEGIN and END macros on uint256 (Wladimir J. van der Laan)

Pull request description:

  Two cleanups in `util/strencodings.h`:

  - Remove `[U](BEGIN|END)` macros — The only use of these was in the Merkle tree code with `uint256` which has its own `begin` and `end` methods which are better.
  - Make ToLower and ToUpper take a char — Unfortunately, `std::string` elements are (bare) chars. As these are the most likely type to be passed to these functions, make them use char instead of unsigned char. This avoids some casts.

Tree-SHA512: 96c8292e1b588d3d7fde95c2e98ad4e7eb75e7baab40a8e8e8209d4e8e7a1bd3b6846601d20976be34a9daabefc50cbc23f3b04200af17d0dfc857c4ec42aca7
@@ -589,7 +589,7 @@ bool ParseHDKeypath(const std::string& keypath_str, std::vector<uint32_t>& keypa

void Downcase(std::string& str)
{
std::transform(str.begin(), str.end(), str.begin(), [](unsigned char c){return ToLower(c);});
std::transform(str.begin(), str.end(), str.begin(), [](char c){return ToLower(c);});

This comment has been minimized.

@laanwj

laanwj Jan 11, 2019

Author Member

Just realizing that it's passing a silly identity function:

std::transform(str.begin(), str.end(), str.begin(), ToLower);

Anyhow, not going to file a PR for just that, I guess the compiler optimizes this away, maybe someone touching this code in the future can take it into account.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment