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

Use explicit casting in cuckoocache's compute_hashes(...) to clarify integer conversion #12770

Merged
merged 1 commit into from Apr 9, 2018
Merged

Use explicit casting in cuckoocache's compute_hashes(...) to clarify integer conversion #12770

merged 1 commit into from Apr 9, 2018

Conversation

practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Mar 23, 2018 •
edited

Use explicit casting in cuckoocache's compute_hashes(...) to clarify integer conversion.

I discussed this code with the code's author @JeremyRubin who suggested patching it to avoid any confusion.

At least one static analyzer incorrectly warns about a shift past bitwidth (UB) here, so this patch will help avoid confusion for human reviewers and static analyzers alike :-)

donaloconnor
donaloconnor approved these changes Mar 23, 2018
View reviewed changes
Copy link
Contributor

@donaloconnor donaloconnor left a comment

Choose a reason for hiding this comment

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

utACK b46d077

@maflcko
Copy link
Member

maflcko commented Mar 23, 2018

b46d077 looks fine, but shouldn't the static analyser be fixed if this is an incorrect warning?

@fanquake
Copy link
Member

@JeremyRubin do you want to ACK this?

@practicalswift
Copy link
Contributor Author

@MarcoFalke Thanks for the utACK. Yes, I'll report this to the static analyser authors.

@maflcko
Copy link
Member

maflcko commented Mar 25, 2018

Thanks. Looks like this can be closed then.

@promag
Copy link
Member

promag commented Mar 25, 2018

utACK b46d077.

@JeremyRubin
Copy link
Contributor

I don't think this changes any behavior, but I think it might be more clear to apply the cast as such:

             (uint32_t)(((uint64_t)hash_function.template operator()<2>(e) * (uint64_t)size) >> 32),

@MarcoFalke I think it's fine to do this even if it is technically a linter bug. It took me a few minutes to read specs of operators, integer conversion hierarchies, and such to be sure that the behavior was correct.

@practicalswift
Copy link
Contributor Author

Updated: Now applying the cast as @JeremyRubin suggested.

Please re-review :-)

@maflcko
Copy link
Member

maflcko commented Apr 1, 2018

utACK 9142dfe

@practicalswift
Copy link
Contributor Author

@JeremyRubin @dcousens @promag @donaloconnor Would you mind re-reviewing? :-)

@donaloconnor
Copy link
Contributor

utACK 9142dfe :-)

@JeremyRubin
Copy link
Contributor

If you check the blame, you actually want @sipa to sign off on this one (he wrote that specific section).

utack otherwise

@maflcko maflcko merged commit 9142dfe into bitcoin:master Apr 9, 2018
maflcko pushed a commit that referenced this pull request Apr 9, 2018
Merge #12770: Use explicit casting in cuckoocache's compute_hashes(..…
603975b 
….) to clarify integer conversion

9142dfe Use explicit casting in cuckoocache's compute_hashes(...) to clarify integer conversion (practicalswift)

Pull request description:

  Use explicit casting in cuckoocache's `compute_hashes(...)` to clarify integer conversion.

  I discussed this code with the code's author @JeremyRubin who suggested patching it to avoid any confusion.

  At least one static analyzer incorrectly warns about a shift past bitwidth (UB) here, so this patch will help avoid confusion for human reviewers and static analyzers alike :-)

Tree-SHA512: 0419ee31b422d2ffedbd1a100688ec0ff5b0c1690d6d92592f638ca8db07a21a9650cb467923108c6f14a38d2bf07d6e6c85d2d1d4b7da53ffe6919f94f32655
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Aug 30, 2019
Merge #12770: Use explicit casting in cuckoocache's compute_hashes(..…
3311745 
….) to clarify integer conversion

Summary:
9142dfe Use explicit casting in cuckoocache's compute_hashes(...) to clarify integer conversion (practicalswift)

Pull request description:

  Use explicit casting in cuckoocache's `compute_hashes(...)` to clarify integer conversion.

  I discussed this code with the code's author @JeremyRubin who suggested patching it to avoid any confusion.

  At least one static analyzer incorrectly warns about a shift past bitwidth (UB) here, so this patch will help avoid confusion for human reviewers and static analyzers alike :-)

Tree-SHA512: 0419ee31b422d2ffedbd1a100688ec0ff5b0c1690d6d92592f638ca8db07a21a9650cb467923108c6f14a38d2bf07d6e6c85d2d1d4b7da53ffe6919f94f32655

Backport of Core PR12770
bitcoin/bitcoin#12770

Test Plan:
  make check
  test_runner.py

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3932
jonspock pushed a commit to jonspock/devault that referenced this pull request Dec 8, 2019
@jonspock
Merge #12770: Use explicit casting in cuckoocache's compute_hashes(..…
5e2ae03 
….) to clarify integer conversion

Summary:
9142dfea81 Use explicit casting in cuckoocache's compute_hashes(...) to clarify integer conversion (practicalswift)

Pull request description:

  Use explicit casting in cuckoocache's `compute_hashes(...)` to clarify integer conversion.

  I discussed this code with the code's author @JeremyRubin who suggested patching it to avoid any confusion.

  At least one static analyzer incorrectly warns about a shift past bitwidth (UB) here, so this patch will help avoid confusion for human reviewers and static analyzers alike :-)

Tree-SHA512: 0419ee31b422d2ffedbd1a100688ec0ff5b0c1690d6d92592f638ca8db07a21a9650cb467923108c6f14a38d2bf07d6e6c85d2d1d4b7da53ffe6919f94f32655

Backport of Core PR12770
bitcoin/bitcoin#12770

Test Plan:
  make check
  test_runner.py

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3932
jonspock pushed a commit to jonspock/devault that referenced this pull request Dec 8, 2019
@jonspock
Merge #12770: Use explicit casting in cuckoocache's compute_hashes(..…
83b75f3 
….) to clarify integer conversion

Summary:
9142dfea81 Use explicit casting in cuckoocache's compute_hashes(...) to clarify integer conversion (practicalswift)

Pull request description:

  Use explicit casting in cuckoocache's `compute_hashes(...)` to clarify integer conversion.

  I discussed this code with the code's author @JeremyRubin who suggested patching it to avoid any confusion.

  At least one static analyzer incorrectly warns about a shift past bitwidth (UB) here, so this patch will help avoid confusion for human reviewers and static analyzers alike :-)

Tree-SHA512: 0419ee31b422d2ffedbd1a100688ec0ff5b0c1690d6d92592f638ca8db07a21a9650cb467923108c6f14a38d2bf07d6e6c85d2d1d4b7da53ffe6919f94f32655

Backport of Core PR12770
bitcoin/bitcoin#12770

Test Plan:
  make check
  test_runner.py

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3932
jonspock pushed a commit to jonspock/devault that referenced this pull request Dec 8, 2019
@jonspock
Merge #12770: Use explicit casting in cuckoocache's compute_hashes(..…
a017a02 
….) to clarify integer conversion

Summary:
9142dfea81 Use explicit casting in cuckoocache's compute_hashes(...) to clarify integer conversion (practicalswift)

Pull request description:

  Use explicit casting in cuckoocache's `compute_hashes(...)` to clarify integer conversion.

  I discussed this code with the code's author @JeremyRubin who suggested patching it to avoid any confusion.

  At least one static analyzer incorrectly warns about a shift past bitwidth (UB) here, so this patch will help avoid confusion for human reviewers and static analyzers alike :-)

Tree-SHA512: 0419ee31b422d2ffedbd1a100688ec0ff5b0c1690d6d92592f638ca8db07a21a9650cb467923108c6f14a38d2bf07d6e6c85d2d1d4b7da53ffe6919f94f32655

Backport of Core PR12770
bitcoin/bitcoin#12770

Test Plan:
  make check
  test_runner.py

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3932
proteanx pushed a commit to devaultcrypto/devault that referenced this pull request Dec 12, 2019
@proteanx
Merge #12770: Use explicit casting in cuckoocache's compute_hashes(..…
b6bb87c 
….) to clarify integer conversion

Summary:
9142dfea81 Use explicit casting in cuckoocache's compute_hashes(...) to clarify integer conversion (practicalswift)

Pull request description:

  Use explicit casting in cuckoocache's `compute_hashes(...)` to clarify integer conversion.

  I discussed this code with the code's author @JeremyRubin who suggested patching it to avoid any confusion.

  At least one static analyzer incorrectly warns about a shift past bitwidth (UB) here, so this patch will help avoid confusion for human reviewers and static analyzers alike :-)

Tree-SHA512: 0419ee31b422d2ffedbd1a100688ec0ff5b0c1690d6d92592f638ca8db07a21a9650cb467923108c6f14a38d2bf07d6e6c85d2d1d4b7da53ffe6919f94f32655

Backport of Core PR12770
bitcoin/bitcoin#12770

Test Plan:
  make check
  test_runner.py

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3932
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Nov 1, 2020
@PastaPastaPasta
Merge bitcoin#12770: Use explicit casting in cuckoocache's compute_ha…
fc00189 
…shes(...) to clarify integer conversion

9142dfe Use explicit casting in cuckoocache's compute_hashes(...) to clarify integer conversion (practicalswift)

Pull request description:

  Use explicit casting in cuckoocache's `compute_hashes(...)` to clarify integer conversion.

  I discussed this code with the code's author @JeremyRubin who suggested patching it to avoid any confusion.

  At least one static analyzer incorrectly warns about a shift past bitwidth (UB) here, so this patch will help avoid confusion for human reviewers and static analyzers alike :-)

Tree-SHA512: 0419ee31b422d2ffedbd1a100688ec0ff5b0c1690d6d92592f638ca8db07a21a9650cb467923108c6f14a38d2bf07d6e6c85d2d1d4b7da53ffe6919f94f32655
@practicalswift practicalswift deleted the cuckoocache branch April 10, 2021 19:34
@str4d str4d mentioned this pull request Apr 20, 2021
Merged
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Feb 5, 2022
Merge bitcoin#12770: Use explicit casting in cuckoocache's compute_ha…
8395d66 
…shes(...) to clarify integer conversion

9142dfe Use explicit casting in cuckoocache's compute_hashes(...) to clarify integer conversion (practicalswift)

Pull request description:

  Use explicit casting in cuckoocache's `compute_hashes(...)` to clarify integer conversion.

  I discussed this code with the code's author @JeremyRubin who suggested patching it to avoid any confusion.

  At least one static analyzer incorrectly warns about a shift past bitwidth (UB) here, so this patch will help avoid confusion for human reviewers and static analyzers alike :-)

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

@dcousens dcousens dcousens approved these changes

@donaloconnor donaloconnor donaloconnor approved these changes

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 @maflcko @fanquake @promag @JeremyRubin @donaloconnor @dcousens