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.

Sign up for GitHub

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

Jump to bottom

Improve readability of DecodeBase58Check(...) #10961

Merged
merged 1 commit into from Oct 9, 2017
Merged

Improve readability of DecodeBase58Check(...) #10961

merged 1 commit into from Oct 9, 2017

Conversation

practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Jul 31, 2017 •
edited

Use the more readable form ...

&vchRet[vchRet.size() - 4]

... instead of ...

&v.end()[-n]

Has the added benefit of eliminating a spurious static analyzer warning about improper use of negative values.

@promag
Copy link
Member

promag commented Jul 31, 2017

utACK 6ab2ce8.

@jonasschnelli
Copy link
Contributor

This seems to be an over-cosmetic change... not sure if its worth.
utACK 6ab2ce83d9268fc81ac3bec41365ce2d2adfd349

laanwj
laanwj reviewed Aug 23, 2017
View reviewed changes
src/base58.cpp Outdated
@@ -136,7 +136,7 @@ bool DecodeBase58Check(const char* psz, std::vector<unsigned char>& vchRet)
}
// re-calculate the checksum, ensure it matches the included 4-byte checksum
uint256 hash = Hash(vchRet.begin(), vchRet.end() - 4);
if (memcmp(&hash, &vchRet.end()[-4], 4) != 0) {
if (memcmp(&hash, vchRet.data() + vchRet.size() - 4, 4) != 0) {
Copy link
Member

Choose a reason for hiding this comment

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

I guess there's some reason vchRet.end() - 4 doesn't work (pointer versus iterator conflict or something like that)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah vchRet.end()-4 would still be an iterator I believe, so you'd have to do some ugly thing like &*(vchRet.end()-4) to make it into a pointer iirc?

Copy link
Member

@sipa sipa Aug 23, 2017 •
edited

Choose a reason for hiding this comment

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

How about &vchRet[vchRet.size() - 4] ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Believe that would work, and yeah that's a lot clearer imo 👍

@practicalswift
Copy link
Contributor Author

@laanwj @meshcollider @sipa Thanks for reviewing! Changed to &vchRet[vchRet.size() - 4]. Looks good? :-)

@meshcollider
Copy link
Contributor

Just confirmed this should be identical behavior, ACK c6a995e

@practicalswift
Copy link
Contributor Author

Travis have completed without errors, but GitHub still reports "1 pending check".

@practicalswift
Copy link
Contributor Author

Seems like Travis CI is stuck. Anyone able to re-start Travis? :-)

ryanofsky
ryanofsky reviewed Aug 29, 2017
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

utACK c6a995e

@sipa
Copy link
Member

sipa commented Aug 29, 2017

utACK c6a995e

@laanwj laanwj merged commit c6a995e into bitcoin:master Oct 9, 2017
laanwj added a commit that referenced this pull request Oct 9, 2017
@laanwj
Merge #10961: Improve readability of DecodeBase58Check(...)
da0478e 
c6a995e Improve readability of DecodeBase58Check(...) (practicalswift)

Pull request description:

  Use the more readable form ...

  ```c++
  &vchRet[vchRet.size() - 4]
  ```

  ... instead of ...

  ```c++
  &v.end()[-n]
  ```

  Has the added benefit of eliminating a spurious static analyzer warning about improper use of negative values.

Tree-SHA512: 5895310c189e9322082c28f34342ff9a6c238e2cae3f204521111c8a7981bc555af60b42de082c91608c1125dfc244a65c4faf929249a067a51435e2be74cb39
@dagurval dagurval mentioned this pull request Dec 28, 2017
Merged
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 31, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#10961: Improve readability of DecodeBase58Check(...)
2cb7e22 
c6a995e Improve readability of DecodeBase58Check(...) (practicalswift)

Pull request description:

  Use the more readable form ...

  ```c++
  &vchRet[vchRet.size() - 4]
  ```

  ... instead of ...

  ```c++
  &v.end()[-n]
  ```

  Has the added benefit of eliminating a spurious static analyzer warning about improper use of negative values.

Tree-SHA512: 5895310c189e9322082c28f34342ff9a6c238e2cae3f204521111c8a7981bc555af60b42de082c91608c1125dfc244a65c4faf929249a067a51435e2be74cb39
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 31, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#10961: Improve readability of DecodeBase58Check(...)
2c12439 
c6a995e Improve readability of DecodeBase58Check(...) (practicalswift)

Pull request description:

  Use the more readable form ...

  ```c++
  &vchRet[vchRet.size() - 4]
  ```

  ... instead of ...

  ```c++
  &v.end()[-n]
  ```

  Has the added benefit of eliminating a spurious static analyzer warning about improper use of negative values.

Tree-SHA512: 5895310c189e9322082c28f34342ff9a6c238e2cae3f204521111c8a7981bc555af60b42de082c91608c1125dfc244a65c4faf929249a067a51435e2be74cb39
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 4, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#10961: Improve readability of DecodeBase58Check(...)
ced1ed1 
c6a995e Improve readability of DecodeBase58Check(...) (practicalswift)

Pull request description:

  Use the more readable form ...

  ```c++
  &vchRet[vchRet.size() - 4]
  ```

  ... instead of ...

  ```c++
  &v.end()[-n]
  ```

  Has the added benefit of eliminating a spurious static analyzer warning about improper use of negative values.

Tree-SHA512: 5895310c189e9322082c28f34342ff9a6c238e2cae3f204521111c8a7981bc555af60b42de082c91608c1125dfc244a65c4faf929249a067a51435e2be74cb39
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 9, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#10961: Improve readability of DecodeBase58Check(...)
8a944a1 
c6a995e Improve readability of DecodeBase58Check(...) (practicalswift)

Pull request description:

  Use the more readable form ...

  ```c++
  &vchRet[vchRet.size() - 4]
  ```

  ... instead of ...

  ```c++
  &v.end()[-n]
  ```

  Has the added benefit of eliminating a spurious static analyzer warning about improper use of negative values.

Tree-SHA512: 5895310c189e9322082c28f34342ff9a6c238e2cae3f204521111c8a7981bc555af60b42de082c91608c1125dfc244a65c4faf929249a067a51435e2be74cb39
@furszy furszy mentioned this pull request Jun 11, 2020
Merged
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Jun 27, 2020
@random-zebra
Merge #1676: [Backport] Base58 performance improvements
961e2d2 
aa633c8 CBase58Data::SetString: cleanse the full vector (Kaz Wesley)
0b77f8a Improve readability of DecodeBase58Check(...) (practicalswift)
dadadee Use prefix operator in for loop of DecodeBase58. (Jiaxing Wang)
305c382 base58: Improve DecodeBase58 performance. (Jiaxing Wang)
3cb418b Improve EncodeBase58 performance (João Barbosa)
4d17f71 don't try to decode invalid encoded ext keys (Jonas Schnelli)
eef4ec6 extend bip32 tests to cover Base58c/CExtKey decode (furszy)
7239aaf fix and extend CBitcoinExtKeyBase template - fix Decode call (req. only one param) - add constructor for base58c->CExtKey (furszy)
13f09c3 remove unused inv from ConnectTip() (furszy)

Pull request description:

  Coming from:

  * bitcoin#6468 .
  * bitcoin#7656 .
  * bitcoin#7922
  * bitcoin#8736 .
  * bitcoin#10961 .

ACKs for top commit:
  random-zebra:
    ACK aa633c8
  Fuzzbawls:
    ACK aa633c8

Tree-SHA512: 3add3106a847b0b3d768df2c4ab5eae7d009e3820998fb5a4cd274169c64622e83ecd14dca51c31f3f6053199834129a1c6920b7ef1193632339297a041173d6
@practicalswift practicalswift deleted the DecodeBase58Check-cleanup branch April 10, 2021 19:32
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Jul 1, 2021
@laanwj @gades
Merge bitcoin#10961: Improve readability of DecodeBase58Check(...)
dcd247b 
c6a995e Improve readability of DecodeBase58Check(...) (practicalswift)

Pull request description:

  Use the more readable form ...

  ```c++
  &vchRet[vchRet.size() - 4]
  ```

  ... instead of ...

  ```c++
  &v.end()[-n]
  ```

  Has the added benefit of eliminating a spurious static analyzer warning about improper use of negative values.

Tree-SHA512: 5895310c189e9322082c28f34342ff9a6c238e2cae3f204521111c8a7981bc555af60b42de082c91608c1125dfc244a65c4faf929249a067a51435e2be74cb39
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Feb 11, 2022
@laanwj
Merge bitcoin#10961: Improve readability of DecodeBase58Check(...)
7f7ff75 
c6a995e Improve readability of DecodeBase58Check(...) (practicalswift)

Pull request description:

  Use the more readable form ...

  ```c++
  &vchRet[vchRet.size() - 4]
  ```

  ... instead of ...

  ```c++
  &v.end()[-n]
  ```

  Has the added benefit of eliminating a spurious static analyzer warning about improper use of negative values.

Tree-SHA512: 5895310c189e9322082c28f34342ff9a6c238e2cae3f204521111c8a7981bc555af60b42de082c91608c1125dfc244a65c4faf929249a067a51435e2be74cb39
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laanwj laanwj laanwj left review comments

@sipa sipa sipa left review comments

@meshcollider meshcollider meshcollider left review comments

@ryanofsky ryanofsky ryanofsky left review comments

Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@practicalswift @promag @jonasschnelli @meshcollider @sipa @laanwj @ryanofsky