consensus/params: simplify ValidDeployment check to avoid gcc warning #22597
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
maflcko
reviewed
Aug 1, 2021
Member
maflcko
left a comment
maflcko
left a comment
There was a problem hiding this comment.
Left a review comment. Also, unrelated:
DeploymentActiveAt could be a template as well:
diff --git a/src/deploymentstatus.h b/src/deploymentstatus.h
index f95c5996f5..f133d6ec6a 100644
--- a/src/deploymentstatus.h
+++ b/src/deploymentstatus.h
@@ -27,13 +27,8 @@ inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus
}
/** Determine if a deployment is active for this block */
-inline bool DeploymentActiveAt(const CBlockIndex& index, const Consensus::Params& params, Consensus::BuriedDeployment dep)
-{
- assert(Consensus::ValidDeployment(dep));
- return index.nHeight >= params.DeploymentHeight(dep);
-}
-
-inline bool DeploymentActiveAt(const CBlockIndex& index, const Consensus::Params& params, Consensus::DeploymentPos dep)
+template <typename Dep>
+bool DeploymentActiveAt(const CBlockIndex& index, const Consensus::Params& params, Dep dep)
{
assert(Consensus::ValidDeployment(dep));
return DeploymentActiveAfter(index.pprev, params, dep);The benefit (apart from less code) would also be a way to enforce that the two function have the same signature. See #22564 (comment)
src/deploymentstatus.cpp
Outdated
Member
There was a problem hiding this comment.
Is there a reason to use three template parameters when a single one would be sufficient?
template <typename X>
static constexpr bool is_minimum(const X x)
{
using U = typename std::underlying_type<X>::type;
return x == std::numeric_limits<U>::min();
}
static_assert(is_minimum(Consensus::DEPLOYMENT_HEIGHTINCB), "heightincb is not minimum value for BuriedDeployment");
static_assert(is_minimum(Consensus::DEPLOYMENT_TESTDUMMY), "testdummy is not minimum value for DeploymentPos");This seems verbose, confusing and dangerous. For example the following asserts are wrong, but don't fail:
static_assert(is_minimum<Consensus::BuriedDeployment, Consensus::BuriedDeployment, signed short>(Consensus::DEPLOYMENT_HEIGHTINCB), "heightincb is not minimum value for BuriedDeployment");
static_assert(is_minimum<Consensus::DeploymentPos, Consensus::DeploymentPos, unsigned short>(Consensus::DEPLOYMENT_TESTDUMMY), "testdummy is not minimum value for DeploymentPos");
Contributor
|
Concept ACK |
Contributor
|
tested ACK 9bf43b09b6efb069e2b2e8551b4c5a63b9348815 It solves the warnings on gcc 8.3.0 (Debian 10). |
tryphe
reviewed
Aug 2, 2021
src/consensus/params.h
Outdated
Contributor
There was a problem hiding this comment.
I think it's important to let other reviewers know that MAX_VERSION_BITS_DEPLOYMENTS is not intended to be assigned, hence we only check if it's less, unlike on line 26 where dep might equal DEPLOYMENT_SEGWIT.
At least that was my interpretation.
ajtowns
force-pushed
the
202108-deploygccwarn
branch
from
9bf43b0 to
0591710
Compare
Member
|
review ACK 0591710 |
Member
|
Added for rc3 (if there is one) |
Contributor
Contributor
Member
|
tested on aarch64: |
maflcko
pushed a commit
to bitcoin-core/gui
that referenced
this pull request
Aug 3, 2021
…nt check to avoid gcc warning 0591710 consensus/params: simplify ValidDeployment check to avoid gcc warning (Anthony Towns) Pull request description: Simplifies the ValidDeployment check to only check the upperbound, relying on the lower bound to be trivially true due to enum values starting at the minimum value of the underlying type (which is checked at compile time in deploymentstatus.cpp). Avoids a "comparison always true" warning in some versions of gcc. Fixes #22587 ACKs for top commit: MarcoFalke: review ACK 0591710 tryphe: retested ACK 0591710 Tree-SHA512: e53b5d478b46d35ec476d004e3c92803afb874c138dd6ef3848861f4281cc113fe88921bc4ac74fd4decbf318ed776d3f816c3a1185f99dc36a5cfecfff51f7c
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 4, 2021
… to avoid gcc warning 0591710 consensus/params: simplify ValidDeployment check to avoid gcc warning (Anthony Towns) Pull request description: Simplifies the ValidDeployment check to only check the upperbound, relying on the lower bound to be trivially true due to enum values starting at the minimum value of the underlying type (which is checked at compile time in deploymentstatus.cpp). Avoids a "comparison always true" warning in some versions of gcc. Fixes bitcoin#22587 ACKs for top commit: MarcoFalke: review ACK 0591710 tryphe: retested ACK 0591710 Tree-SHA512: e53b5d478b46d35ec476d004e3c92803afb874c138dd6ef3848861f4281cc113fe88921bc4ac74fd4decbf318ed776d3f816c3a1185f99dc36a5cfecfff51f7c
Merged
Member
|
Backported in #22629. |
hebasto
pushed a commit
to hebasto/bitcoin
that referenced
this pull request
Aug 6, 2021
Github-Pull: bitcoin#22597 Rebased-From: 0591710
hebasto
pushed a commit
to hebasto/bitcoin
that referenced
this pull request
Aug 6, 2021
Github-Pull: bitcoin#22597 Rebased-From: 0591710
hebasto
pushed a commit
to hebasto/bitcoin
that referenced
this pull request
Aug 6, 2021
Github-Pull: bitcoin#22597 Rebased-From: 0591710
SmartArray
pushed a commit
to JaredTate/digibyte
that referenced
this pull request
Aug 7, 2021
Github-Pull: bitcoin/bitcoin#22597 Rebased-From: 0591710
hebasto
pushed a commit
to hebasto/bitcoin
that referenced
this pull request
Aug 7, 2021
Github-Pull: bitcoin#22597 Rebased-From: 0591710
hebasto
pushed a commit
to hebasto/bitcoin
that referenced
this pull request
Aug 9, 2021
Github-Pull: bitcoin#22597 Rebased-From: 0591710
hebasto
pushed a commit
to hebasto/bitcoin
that referenced
this pull request
Aug 9, 2021
Github-Pull: bitcoin#22597 Rebased-From: 0591710
hebasto
pushed a commit
to hebasto/bitcoin
that referenced
this pull request
Aug 20, 2021
Github-Pull: bitcoin#22597 Rebased-From: 0591710
hebasto
pushed a commit
to hebasto/bitcoin
that referenced
this pull request
Aug 20, 2021
Github-Pull: bitcoin#22597 Rebased-From: 0591710
hebasto
pushed a commit
to hebasto/bitcoin
that referenced
this pull request
Aug 20, 2021
Github-Pull: bitcoin#22597 Rebased-From: 0591710
laanwj
added a commit
that referenced
this pull request
Aug 26, 2021
32e1424 Fix build with Boost 1.77.0 (Rafael Sadowski) cb34a0a qt: Handle new added plurals in bitcoin_en.ts (Hennadii Stepanov) 068985c doc: Mention the flat directory structure for uploads (Andrew Chow) 27d43e5 guix: Don't include directory name in SHA256SUMS (Andrew Chow) 88fb7e3 test: fix bug in 22686 (S3RK) 63fec7e clientversion: No suffix #if CLIENT_VERSION_IS_RELEASE (Carl Dong) dfaffbe test: Test for ApproximateBestSubset edge case with too little fees (Andrew Chow) e86b023 wallet: Assert that enough was selected to cover the fees (Andrew Chow) ffc81e2 wallet: Use GetSelectionAmount for target value calculations (Andrew Chow) ce77b45 release: Release with separate SHA256SUMS and sig files (Carl Dong) cb491bd guix-verify: Non-zero exit code when anything fails (Carl Dong) 6a611d2 gui: ensure external signer option remains disabled without signers (Andrew Chow) e9b4487 qt: Fix regression in "Encrypt Wallet" menu item (Hennadii Stepanov) 57fce06 consensus/params: simplify ValidDeployment check to avoid gcc warning (Anthony Towns) e9d30fb ci: Run fuzzer task for the master branch only (Hennadii Stepanov) Pull request description: Backported: 1) #22730 1) bitcoin-core/gui#393 1) #22597 1) bitcoin-core/gui#396 1) #22643 1) #22642 1) #22685 1) #22686 1) #22654 1) #22742 1) bitcoin-core/gui#406 1) #22713 ACKs for top commit: laanwj: Code list-of-backported-PRs review ACK 32e1424 Tree-SHA512: f5e2dd1be6cdcd39368eeb5d297b3ff4418d0bf2e70c90e59ab4ba1dbf16f773045d877b4997511de58c3aca75a978dcf043e338bad23951557e2a27ccc845f6
fujicoin
pushed a commit
to fujicoin/fujicoin-22.0
that referenced
this pull request
Aug 27, 2021
Github-Pull: bitcoin/bitcoin#22597 Rebased-From: 059171009b0138555f311cedc2553015ff618323
gwillen
pushed a commit
to gwillen/elements
that referenced
this pull request
Jul 27, 2022
Github-Pull: bitcoin/bitcoin#22597 Rebased-From: 0591710
gwillen
pushed a commit
to gwillen/elements
that referenced
this pull request
Aug 1, 2022
Github-Pull: bitcoin/bitcoin#22597 Rebased-From: 0591710
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
8 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Simplifies the ValidDeployment check to only check the upperbound, relying on the lower bound to be trivially true due to enum values starting at the minimum value of the underlying type (which is checked at compile time in deploymentstatus.cpp). Avoids a "comparison always true" warning in some versions of gcc.
Fixes #22587