Don't make "in" parameters look like "out"/"in-out" parameters: pass by ref to const instead of ref to non-const

practicalswift commented at 0:30 am on December 6, 2020: contributor

Don’t make “in” parameters look like “out”/“in-out” parameters: pass by ref to const instead of ref to non-const.

Don't make "in" parameters look like "out"/"in-out" parameters: pass by ref to const instead of ref to non-const 12dcdaaa54

fanquake added the label Refactoring on Dec 6, 2020

DrahtBot commented at 3:36 am on December 6, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18479 (RPC: Show fee in results for signrawtransaction* for segwit inputs by luke-jr)

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.

DrahtBot commented at 10:14 am on December 6, 2020: member

🕵️ @sipa @fjahr have been requested to review this pull request as specified in the REVIEWERS file.

MarcoFalke commented at 10:30 am on December 6, 2020: member

cr ACK 12dcdaaa54b9a4c55384e6b04379da8125fa6e7d

hebasto approved

hebasto commented at 11:16 am on December 6, 2020: member

ACK 12dcdaaa54b9a4c55384e6b04379da8125fa6e7d, I have reviewed the code and it looks OK, I agree it can be merged.

All findings are made by a cool static analyzer, aren’t they?

practicalswift commented at 11:56 am on December 6, 2020: contributor

All findings are made by a cool static analyzer, aren’t they?

Yes, cppcheck should have the credit for these findings :)

To reproduce:

 0$ CPPCHECK_VERSION=2.3
 1$ curl -s https://codeload.github.com/danmar/cppcheck/tar.gz/${CPPCHECK_VERSION} | tar -zxf - --directory /tmp/
 2$ (cd /tmp/cppcheck-${CPPCHECK_VERSION}/ && make CFGDIR=/tmp/cppcheck-${CPPCHECK_VERSION}/cfg/ > /dev/null)
 3$ /tmp/cppcheck-${CPPCHECK_VERSION}/cppcheck \
 4    --enable=all \
 5    --language=c++ \
 6    --std=c++17 \
 7    -D__cplusplus \
 8    -DCLIENT_VERSION_BUILD \
 9    -DCLIENT_VERSION_IS_RELEASE \
10    -DCLIENT_VERSION_MAJOR \
11    -DCLIENT_VERSION_MINOR \
12    -DCLIENT_VERSION_REVISION \
13    -DCOPYRIGHT_YEAR \
14    -DDEBUG \
15    -DHAVE_WORKING_BOOST_SLEEP_FOR \
16    -I src/ \
17    -q \
18    $(git ls-files -- "*.cpp" "*.h" ":(exclude)src/leveldb/" ":(exclude)src/secp256k1/" ":(exclude)src/univalue/") 2>&1 | \
19    grep constParameter
20src/init.cpp:917:37: style: Parameter 'args' can be declared with const [constParameter]
21src/node/coinstats.cpp:115:55: style: Parameter 'stats' can be declared with const [constParameter]
22src/pubkey.cpp:253:55: style: Parameter 'ccChild' can be declared with const [constParameter]
23src/rpc/rawtransaction_util.cpp:289:143: style: Parameter 'input_errors' can be declared with const [constParameter]
24src/script/sigcache.cpp:49:32: style: Parameter 'entry' can be declared with const [constParameter]
25src/script/sigcache.cpp:56:34: style: Parameter 'entry' can be declared with const [constParameter]
26src/script/sigcache.cpp:69:23: style: Parameter 'entry' can be declared with const [constParameter]
27src/test/addrman_tests.cpp:74:32: style: Parameter 'addr' can be declared with const [constParameter]
28src/test/net_tests.cpp:78:63: style: Parameter '_addrman' can be declared with const [constParameter]
29src/test/util_tests.cpp:1863:48: style: Parameter 'span' can be declared with const [constParameter]
30src/validation.cpp:924:67: style: Parameter 'ws' can be declared with const [constParameter]
31src/validation.cpp:951:70: style: Parameter 'ws' can be declared with const [constParameter]
32src/wallet/feebumper.cpp:114:115: style: Parameter 'coin_control' can be declared with const [constParameter]
33src/wallet/interfaces.cpp:80:44: style: Parameter 'wallet' can be declared with const [constParameter]
34src/wallet/interfaces.cpp:97:38: style: Parameter 'wallet' can be declared with const [constParameter]

fjahr commented at 1:03 pm on December 6, 2020: member

Code review ACK 12dcdaaa54b9a4c55384e6b04379da8125fa6e7d

luke-jr approved

luke-jr commented at 5:00 pm on December 6, 2020: member

utACK

MarcoFalke merged this on Dec 6, 2020

MarcoFalke closed this on Dec 6, 2020

sidhujag referenced this in commit 7aaed70bf8 on Dec 6, 2020

MarcoFalke referenced this in commit f13e03cda2 on Jan 7, 2021

in src/validation.cpp:508 in 12dcdaaa54

504@@ -505,13 +505,13 @@ class MemPoolAccept
505 
506     // Run the script checks using our policy flags. As this can be slow, we should
507     // only invoke this on transactions that have otherwise passed policy checks.
508-    bool PolicyScriptChecks(ATMPArgs& args, Workspace& ws, PrecomputedTransactionData& txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
509+    bool PolicyScriptChecks(ATMPArgs& args, const Workspace& ws, PrecomputedTransactionData& txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

glozow commented at 11:22 pm on January 18, 2021:

Er, would it be alright if I changed the Workspace here back to non-const? I’d interpret Workspace as an “in-out” parameter used throughout MemPoolAccept::AcceptSingleTransaction. As I understand it, this PR makes it const for 2 of the intermediate steps based on some code analysis, but I think it might just be coincidentally unmodified inside those functions 😅 I’d like to be able to update the workspace inside PolicyScriptChecks/ConsensusScriptChecks.

MarcoFalke commented at 6:11 am on January 19, 2021:

Sure, just remove the const again. While the changes here are correct (as in adding a const will still compile), they probably don’t make sense conceptually.

practicalswift commented at 9:24 am on January 19, 2021:

@glozow Absolutely! What is const is not necessarily const over time :)

The C++ const correctness FAQ (isocpp.org) is great reading on the topic!

glozow commented at 12:00 pm on January 19, 2021:

Coolio, thanks 🤗

practicalswift deleted the branch on Apr 10, 2021

Fabcien referenced this in commit a95da7d320 on Feb 24, 2022

kittywhiskers referenced this in commit 7daf4023e0 on Jul 18, 2022

kittywhiskers referenced this in commit 979a7ecc0b on Aug 9, 2022

DrahtBot locked this on Aug 16, 2022

Don’t make “in” parameters look like “out”/“in-out” parameters: pass by ref to const instead of ref to non-const #20581

Conflicts