Skip to content

refactor, bench, fuzz: Drop unneeded UCharCast calls #29820

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
Merged
merged 1 commit into from
Apr 9, 2024
Merged

refactor, bench, fuzz: Drop unneeded UCharCast calls #29820

merged 1 commit into from
Apr 9, 2024

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Apr 6, 2024

The CKey::Set() template function handles std::byte just fine:

bitcoin/src/key.h

Line 105 in b5d2118

memcpy(keydata->data(), (unsigned char*)&pbegin[0], keydata->size());

Noticed in #29774 (comment).

@hebasto
refactor, bench, fuzz: Drop unneeded UCharCast calls
56e1e5d 
The `CKey::Set()` template function handles `std::byte` just fine.
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 6, 2024 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK maflcko, laanwj, hernanmarino

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

@hebasto hebasto mentioned this pull request Apr 6, 2024
Merged
@maflcko
Copy link
Member

maflcko commented Apr 7, 2024

lgtm ACK 56e1e5d

A (more invasive) alternative would be to use Set(Span<std::byte>), or Set(Span<BasicByte>)

@laanwj
Copy link
Member

laanwj commented Apr 9, 2024

Seems fine, code review ACK 56e1e5d

@hernanmarino
Copy link
Contributor

ACK 56e1e5d

@fanquake fanquake merged commit e319569 into bitcoin:master Apr 9, 2024
@hebasto hebasto deleted the 240406-uchar branch April 10, 2024 06:14
@maflcko
Copy link
Member

maflcko commented Apr 10, 2024

Just for reference, the reason this is safe, is not because of the line linked to in the pull request description, but due to the existing UCharCast in the function. Error when trying to pass the wrong type:

./key.h:103:26: error: no matching function for call to 'UCharCast'
  103 |         } else if (Check(UCharCast(&pbegin[0]))) {
      |                          ^~~~~~~~~
note: in instantiation of function template specialization 'CKey::Set<__gnu_cxx::__normal_iterator<int *, std::vector<int>>>' requested here

Pttn added a commit to RiecoinTeam/Riecoin that referenced this pull request Apr 13, 2024
@Pttn
Merge bitcoin#29820
c02fb99
@bitcoin bitcoin locked and limited conversation to collaborators Apr 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hebasto @DrahtBot @maflcko @laanwj @hernanmarino @fanquake