scripted-diff: Rename overloaded int GetArg to GetIntArg #22976
Merged
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
|
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. |
This was referenced Sep 15, 2021
Closed
|
Inspected the binary to check that only the right calls (with second argument git diff --word-diff-regex=. -U0 -- /tmp/{old,new}/d_objdump |
|
review ACK 5b47496 💜 Show signature and timestampSignature: Timestamp of file with hash |
fanquake
reviewed
Sep 23, 2021
src/init.cpp
Outdated
| @@ -217,7 +217,7 @@ void Shutdown(NodeContext& node) | |||
| node.banman.reset(); | |||
| node.addrman.reset(); | |||
|
|
|||
| if (node.mempool && node.mempool->IsLoaded() && node.args->GetArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) { | |||
| if (node.mempool && node.mempool->IsLoaded() && node.args->GetIntArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) { | |||
There was a problem hiding this comment.
@MarcoFalke's #23061 changes this to GetBoolArg. Given the ACK there, I assume that is the more correct change? If that is going to be backported then it can be merged first and this PR rebased on top.
There was a problem hiding this comment.
(I would expect so too as DEFAULT_PERSIST_MEMPOOL is a boolean constant.)
|
ACK 5b47496 |
kiminuo
approved these changes
Sep 23, 2021
There was a problem hiding this comment.
utACK 5b47496
Went line by line and the patch looks correct to me. https://github.com/bitcoin/bitcoin/pull/22976/files#r714600644 seems good to address though.
| @@ -1217,7 +1217,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) | |||
| if (!ignores_incoming_txs) node.fee_estimator = std::make_unique<CBlockPolicyEstimator>(); | |||
|
|
|||
| assert(!node.mempool); | |||
| int check_ratio = std::min<int>(std::max<int>(args.GetArg("-checkmempool", chainparams.DefaultConsistencyChecks() ? 1 : 0), 0), 1000000); | |||
| int check_ratio = std::min<int>(std::max<int>(args.GetIntArg("-checkmempool", chainparams.DefaultConsistencyChecks() ? 1 : 0), 0), 1000000); | |||
There was a problem hiding this comment.
(Not for this PR but looking at the line I would expect std::clamp here)
edit: Addressed by #23095
| @@ -1263,7 +1263,7 @@ void InitScriptExecutionCache() { | |||
| g_scriptExecutionCacheHasher.Write(nonce.begin(), 32); | |||
| // nMaxCacheSize is unsigned. If -maxsigcachesize is set to zero, | |||
| // setup_bytes creates the minimum possible cache (2 elements). | |||
| size_t nMaxCacheSize = std::min(std::max((int64_t)0, gArgs.GetArg("-maxsigcachesize", DEFAULT_MAX_SIG_CACHE_SIZE) / 2), MAX_MAX_SIG_CACHE_SIZE) * ((size_t) 1 << 20); | |||
| size_t nMaxCacheSize = std::min(std::max((int64_t)0, gArgs.GetIntArg("-maxsigcachesize", DEFAULT_MAX_SIG_CACHE_SIZE) / 2), MAX_MAX_SIG_CACHE_SIZE) * ((size_t) 1 << 20); | |||
There was a problem hiding this comment.
(Not for this PR but looking at the line I would expect std::clamp here)
edit: Addressed by #23095
| @@ -2517,7 +2517,7 @@ bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr | |||
|
|
|||
| CBlockIndex *pindexMostWork = nullptr; | |||
| CBlockIndex *pindexNewTip = nullptr; | |||
| int nStopAtHeight = gArgs.GetArg("-stopatheight", DEFAULT_STOPATHEIGHT); | |||
| int nStopAtHeight = gArgs.GetIntArg("-stopatheight", DEFAULT_STOPATHEIGHT); | |||
There was a problem hiding this comment.
Not for this PR but isn't it better to cast it explicitely here? It's very easy to think that GetIntArg returns int and not int64_t.
5b47496 to
89b96f9
Compare
|
Rebased 5b47496 -> 89b96f9 ( |
|
Concept ACK. |
hebasto
reviewed
Sep 25, 2021
There was a problem hiding this comment.
Suggesting script change:
@@ -7,5 +7,5 @@
with compile time checking.
-BEGIN VERIFY SCRIPT-
-git grep -l GetArg | xargs sed -i 's/GetArg(\([^)]*\( [0-9]\+\|-1\|port\|BaseParams().RPCPort()\|Params().GetDefaultPort()\|_TIMEOUT\|Height\|_WORKQUEUE\|_THREADS\|_MEMPOOL\|_CONNECTIONS\|LIMIT\|SigOp\|Bytes\|_SIZE\|_VERSION\|_AGE\|_CHECKS\|Checks() ? 1 : 0\|_BANTIME\|Cache\|BLOCKS\|LEVEL\|Weight\|Version\|BUFFER\|TARGET\|WEIGHT\|TXN\|TRANSACTIONS\|ADJUSTMENT\|i64\|Size\|nDefault\|_EXPIRY\|HEIGHT\|SIZE\|SNDHWM\|_TIME_MS\)\))/GetIntArg(\1)/g'
+git grep -l GetArg | xargs sed -i 's/GetArg(\([^)]*\( [0-9]\+\|-1\|port\|BaseParams().RPCPort()\|Params().GetDefaultPort()\|_TIMEOUT\|Height\|_WORKQUEUE\|_THREADS\|_CONNECTIONS\|LIMIT\|SigOp\|Bytes\|_VERSION\|_AGE\|_CHECKS\|Checks() ? 1 : 0\|_BANTIME\|Cache\|BLOCKS\|LEVEL\|Weight\|Version\|BUFFER\|TARGET\|WEIGHT\|TXN\|TRANSACTIONS\|ADJUSTMENT\|i64\|Size\|nDefault\|_EXPIRY\|HEIGHT\|SIZE\|SNDHWM\|_TIME_MS\)\))/GetIntArg(\1)/g'
-END VERIFY SCRIPT-
This was referenced Sep 25, 2021
Improve readability of code, simplify future scripted diff cleanup PRs, and be more consistent with naming for GetBoolArg. This will also be useful for replacing runtime settings type checking with compile time checking. -BEGIN VERIFY SCRIPT- git grep -l GetArg | xargs sed -i 's/GetArg(\([^)]*\( [0-9]\+\|-1\|port\|BaseParams().RPCPort()\|Params().GetDefaultPort()\|_TIMEOUT\|Height\|_WORKQUEUE\|_THREADS\|_CONNECTIONS\|LIMIT\|SigOp\|Bytes\|_VERSION\|_AGE\|_CHECKS\|Checks() ? 1 : 0\|_BANTIME\|Cache\|BLOCKS\|LEVEL\|Weight\|Version\|BUFFER\|TARGET\|WEIGHT\|TXN\|TRANSACTIONS\|ADJUSTMENT\|i64\|Size\|nDefault\|_EXPIRY\|HEIGHT\|SIZE\|SNDHWM\|_TIME_MS\)\))/GetIntArg(\1)/g' -END VERIFY SCRIPT- Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
89b96f9 to
93b9800
Compare
|
Rebased 89b96f9 -> 93b9800 ( |
hebasto
approved these changes
Sep 27, 2021
maflcko
approved these changes
Sep 27, 2021
There was a problem hiding this comment.
re-ACK 93b9800 📨
Show signature and timestamp
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
re-ACK 93b9800fec588fc0c258e754405db1e9f89cfb81 📨
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhsoQv+NLfJRdSCRTCqPhR9Dep3iK1duj4XvvoYRP89eikIjevfNPM3bRCXEm0Q
rgyEaxfo5QdvbOZU9gwLjCFUbYGrkNAD1m66LRBf71Bqk+5FJ+wZHDo6cd/RpRxZ
Ijv73U9D1L/bI33ftzVc8yrRCLzU7oYuxgSwX9/OxJqmm1GfdQ4TsJ3xwtX9aBz9
etqr5pX+iTPNfGPMKbKci6qGCkUys8ANt7XoNV8O92cczq9IPlQrau0YJ81Mulaw
jwWn48I3P1gMHx4y9MoWl1xH+0ddoBnGI4PyVwKECUBPac//jZspAkp5PtF3oF8l
4/ejTHzDeA84oanUoAEq3jMbv0dleBxdQkVDB4fs7+JhIxCgW5H7IjSgtlU3G07r
POmz9Y6N/5JG1c22T9tPn385M5pCda2LTfXawusRPuLzCtcrvOC1JnfU/MbKfkvX
zPfVFe8Vt/2W9VKGnOjvkTtQCgwLMOHEc9OjTSOSBL+GoffQrBUwN8KOUVRewCtX
PYZLdVyl
=6CRv
-----END PGP SIGNATURE-----
Timestamp of file with hash 6a0788037cf2c7d2a33005e2ca5b19d4e0ecf44890a2d5133d708fd3c90446a9 -
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Sep 27, 2021
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Sep 12, 2022
Summary: This is a backport of [[bitcoin/bitcoin#22976 | core#22976]] Depends on D11944 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11947
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This is meant to improve readability of code and remove guesswork needed to determine argument types and migrate to typed arguments (#22978) by having distinctly named
GetArgGetArgsGetBoolArgandGetIntArgmethods.This commit was originally part of #22766 and had some review discussion there. But it was wisely suggested to be split off to make that PR smaller.