Pulls down the latest version of https://github.com/llvm/llvm-project/blob/main/compiler-rt/include/fuzzer/FuzzedDataProvider.h, after https://github.com/llvm/llvm-project/pull/177794 was merged upstream.
just a note that in C++20 unsigned‑to‑signed conversion is no longer implementation defined so this is effectively a no‑op in Core and this is mainly an upstream cleanup update
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34396.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | fanquake |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Please push these upstream to https://github.com/llvm/llvm-project/blob/main/compiler-rt/include/fuzzer/FuzzedDataProvider.h
Oh okay thanks…
Opened a PR: https://github.com/llvm/llvm-project/pull/177794
This has been merged upstream.
Can you update the commit message and PR description to something like “fuzz: pull the latest FuzzedDataProvider.h from upstream”. As well as check that there aren’t any other changes that need pulling.
Thank you for the title change..I will make change to commit after some hours
Can you update the commit message and PR description to something like “fuzz: pull the latest FuzzedDataProvider.h from upstream”. As well as check that there aren’t any other changes that need pulling.
Updated the commit message
post-merge ACK dfb93646093f8d71be455fc95e2e06ff73fb9211
Makes the unsigned-to-signed comparison explicit, switches to if constexpr + removes some leftover TODOs.
Current https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/FuzzedDataProvider.h is exactly the same as https://raw.githubusercontent.com/llvm/llvm-project/refs/heads/main/compiler-rt/include/fuzzer/FuzzedDataProvider.h (except for the trailing newline).
Note: my understanding is that the updated code is most likely dead in Core since unsigned-to-signed conversion is no longer implementation-defined in C++20 according to https://en.cppreference.com/w/cpp/language/implicit_conversion.html#Integral_conversions:
If the destination type is signed, the value does not change if the source integer can be represented in the destination type. Otherwise the result is implementation-defined(until C++20)the unique value of the destination type equal to the source value modulo 2n where n is the number of bits used to represent the destination type(since C++20) (note that this is different from signed integer arithmetic overflow, which is undefined).
But since this has to match upstream, it’s fine - but I think it should have been mentioned in the PR. Q: I understand that this isn’t a risky change, but was it urgent that we had to merge it without any other ACKs in this repo?
post-merge ACK dfb9364
Makes the unsigned-to-signed comparison explicit, switches to
if constexpr+ removes some leftover TODOs.Current https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/FuzzedDataProvider.h is exactly the same as https://raw.githubusercontent.com/llvm/llvm-project/refs/heads/main/compiler-rt/include/fuzzer/FuzzedDataProvider.h (except for the trailing newline).
Note: my understanding is that the updated code is most likely dead in Core since unsigned-to-signed conversion is no longer implementation-defined in C++20 according to https://en.cppreference.com/w/cpp/language/implicit_conversion.html#Integral_conversions:
If the destination type is signed, the value does not change if the source integer can be represented in the destination type. Otherwise the result is implementation-defined(until C++20)the unique value of the destination type equal to the source value modulo 2n where n is the number of bits used to represent the destination type(since C++20) (note that this is different from signed integer arithmetic overflow, which is undefined).
But since this has to match upstream, it’s fine - but I think it should have been mentioned in the PR.
Updated the PR description to call out the C++20 conversion semantics and that this is mainly an upstream cleanup sync. Thanks for the review
Labels
Refactoring