-
Notifications
You must be signed in to change notification settings - Fork 37.7k
RFC: Add operator""_uint256
compile-time user-defined literal
#31991
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
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31991. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
6a07360
to
b340e03
Compare
_hex
to uint256
and remove consteval_ctor
operator""_uint256
compile-time user-defined literal
b340e03
to
fbbdbaa
Compare
fbbdbaa
to
1d06571
Compare
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
1d06571
to
8c27aaf
Compare
I think the upstream bug link may be wrong. https://developercommunity.visualstudio.com/t/Can-not-initialize-array-with-consteval/10828473 looks closer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tend toward NACK for now, as I don't see the goal or benefit
Would be happy to ack a doc fix of the upstream msvc bug report
@@ -88,17 +88,17 @@ class CMainParams : public CChainParams { | |||
consensus.signet_challenge.clear(); | |||
consensus.nSubsidyHalvingInterval = 210000; | |||
consensus.script_flag_exceptions.emplace( // BIP16 exception | |||
uint256{"00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22"}, SCRIPT_VERIFY_NONE); | |||
"00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22"_uint256, SCRIPT_VERIFY_NONE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what the goal here is, or the improvement. Looks like extra code is added to do the same thing in two different ways?
Yes, it seems like our issue was closer this "sub-issue" in the currently linked issue: https://developercommunity.visualstudio.com/t/consteval-conversion-function-fails/1579014#T-N10550785 and they apparently only fixed the primary issue. :\ |
Follow-up to Add mainnet assumeutxo param at height 880,000, looking to simplify specifying explicit hashes in the source code.
It's not a full implementation yet, I'm looking for concept ack/nack and full CI feedback for now - applying it to the whole codebase may follow based on that.
This is also a follow-up to refactor: Replace ParseHex with consteval ""_hex literals, meant to simplify the usages (values beginning with the hexadecimal data, followed by the type conversion, similarly to
sizeof(uint256)
vsuint256::size()
) and unifying hex parsing code.Similarly to
""_hex
, the""_uint256
has the same compile-time constraints as its constructor (properly sized with actual hexadecimal values), but usesdetail::Hex
parser (reversed) (used in most other places for hex parsing) instead ofbase_blob(std::string_view hex_str)
.The new implementation also makes it obvious that the values are reversed in this case.
I thought of removing the
consteval_ctor
hack as well, now that consteval conversion is claimed to work in visual studio, but since https://godbolt.org/z/j837req5h still seems to fail for latestx64 msvc v19
, I postponed doing that.