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
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
21+/* ValidDeployment only checks upper bounds for ensuring validity.
22+ * This checks that the lowest possible value or the type is also a
23+ * (specific) valid deployment so that lower bounds don't need to be checked.
24+ */
25+
26+template<typename T, typename X, typename U=typename std::underlying_type<T>::type>
Is there a reason to use three template parameters when a single one would be sufficient?
0template <typename X>
1static constexpr bool is_minimum(const X x)
2{
3 using U = typename std::underlying_type<X>::type;
4 return x == std::numeric_limits<U>::min();
5}
6
7static_assert(is_minimum(Consensus::DEPLOYMENT_HEIGHTINCB), "heightincb is not minimum value for BuriedDeployment");
8static_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:
0static_assert(is_minimum<Consensus::BuriedDeployment, Consensus::BuriedDeployment, signed short>(Consensus::DEPLOYMENT_HEIGHTINCB), "heightincb is not minimum value for BuriedDeployment");
1static_assert(is_minimum<Consensus::DeploymentPos, Consensus::DeploymentPos, unsigned short>(Consensus::DEPLOYMENT_TESTDUMMY), "testdummy is not minimum value for DeploymentPos");
Left a review comment. Also, unrelated:
DeploymentActiveAt could be a template as well:
0diff --git a/src/deploymentstatus.h b/src/deploymentstatus.h
1index f95c5996f5..f133d6ec6a 100644
2--- a/src/deploymentstatus.h
3+++ b/src/deploymentstatus.h
4@@ -27,13 +27,8 @@ inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus
5 }
6
7 /** Determine if a deployment is active for this block */
8-inline bool DeploymentActiveAt(const CBlockIndex& index, const Consensus::Params& params, Consensus::BuriedDeployment dep)
9-{
10- assert(Consensus::ValidDeployment(dep));
11- return index.nHeight >= params.DeploymentHeight(dep);
12-}
13-
14-inline bool DeploymentActiveAt(const CBlockIndex& index, const Consensus::Params& params, Consensus::DeploymentPos dep)
15+template <typename Dep>
16+bool DeploymentActiveAt(const CBlockIndex& index, const Consensus::Params& params, Dep dep)
17 {
18 assert(Consensus::ValidDeployment(dep));
19 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 (review)
tested ACK 9bf43b09b6efb069e2b2e8551b4c5a63b9348815
It solves the warnings on gcc 8.3.0 (Debian 10).
31 DEPLOYMENT_TAPROOT, // Deployment of Schnorr/Taproot (BIPs 340-342)
32 // NOTE: Also add new deployments to VersionBitsDeploymentInfo in deploymentinfo.cpp
33 MAX_VERSION_BITS_DEPLOYMENTS
34 };
35-constexpr bool ValidDeployment(DeploymentPos dep) { return DEPLOYMENT_TESTDUMMY <= dep && dep <= DEPLOYMENT_TAPROOT; }
36+constexpr bool ValidDeployment(DeploymentPos dep) { return dep < MAX_VERSION_BITS_DEPLOYMENTS; }
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.
retested ACK 059171009b0138555f311cedc2553015ff618323 approach ACK 059171009b0138555f311cedc2553015ff618323
gcc 8.3.0
tested on aarch64:
0gcc (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 7.5.0
Labels
Refactoring
Milestone
22.0