Don't make "in" parameters look like "out"/"in-out" parameters: pass by ref to const instead of ref to non-const by practicalswift · Pull Request #20581 · bitcoin/bitcoin

practicalswift · 2020-12-06T00:30:51Z

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

…by ref to const instead of ref to non-const

DrahtBot · 2020-12-06T03:36:56Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

RPC: Show fee in results for signrawtransaction* for segwit inputs #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 · 2020-12-06T10:14:40Z

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

maflcko · 2020-12-06T10:30:29Z

cr ACK 12dcdaa

hebasto

ACK 12dcdaa, 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 · 2020-12-06T11:56:23Z

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

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

To reproduce:

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

fjahr · 2020-12-06T13:03:47Z

Code review ACK 12dcdaa

luke-jr

utACK

…ut" par…

…ions as const 31b136e Don't declare de facto const reference variables as non-const (practicalswift) 1c65c07 Don't declare de facto const member functions as non-const (practicalswift) Pull request description: _Meta: This is the second and final part of the `const` refactoring series (part one: #20581). **I promise: no more refactoring PRs from me in a while! :)** I'll now go back to focusing on fuzzing/hardening!_ Changes in this PR: * Don't declare de facto const member functions as non-const * Don't declare de facto const reference variables as non-const Awards for finding candidates for the above changes go to: * `clang-tidy`'s [`readability-make-member-function-const`](https://clang.llvm.org/extra/clang-tidy/checks/readability-make-member-function-const.html) check ([list of `clang-tidy` checks](https://clang.llvm.org/extra/clang-tidy/checks/list.html)) * `cppcheck`'s `constVariable` check ([list of `cppcheck` checks](https://sourceforge.net/p/cppcheck/wiki/ListOfChecks/)) See #18920 for instructions on how to analyse Bitcoin Core using Clang Static Analysis, `clang-tidy` and `cppcheck`. ACKs for top commit: ajtowns: ACK 31b136e jonatack: ACK 31b136e theStack: ACK 31b136e ❄️ Tree-SHA512: f58f8f00744219426874379e9f3e9331132b9b48e954d24f3a85cbb858fdcc98009ed42ef7e7b4619ae8af9fc240a6d8bfc1c438db2e97b0ecd722a80dcfeffe

glozow · 2021-01-18T23:22:45Z

src/validation.cpp

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


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.

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.

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

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

Coolio, thanks 🤗

…by ref to const instead of ref to non-const Summary: This is a backport of [[bitcoin/bitcoin#20581 | core#20581]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11094

…ut" parameters: pass by ref to const instead of ref to non-const

Don't make "in" parameters look like "out"/"in-out" parameters: pass …

12dcdaa

…by ref to const instead of ref to non-const

fanquake added the Refactoring label Dec 6, 2020

DrahtBot mentioned this pull request Dec 6, 2020

RPC: Show fee in results for signrawtransaction* for segwit inputs #18479

Closed

hebasto approved these changes Dec 6, 2020

View reviewed changes

luke-jr approved these changes Dec 6, 2020

View reviewed changes

maflcko merged commit 9385549 into bitcoin:master Dec 6, 2020

practicalswift mentioned this pull request Dec 6, 2020

Declare de facto const reference variables/member functions as const #20584

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 6, 2020

Merge bitcoin#20581: Don't make "in" parameters look like "out"/"in-o…

7aaed70

…ut" par…

glozow reviewed Jan 18, 2021

View reviewed changes

practicalswift deleted the const-ref-parameters branch April 10, 2021 19:43

kwvg added a commit to kwvg/dash that referenced this pull request Jul 18, 2022

merge bitcoin#20581: Don't make "in" parameters look like "out"/"in-o…

7daf402

…ut" parameters: pass by ref to const instead of ref to non-const

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Don't make "in" parameters look like "out"/"in-out" parameters: pass by ref to const instead of ref to non-const#20581
maflcko merged 1 commit intobitcoin:masterfrom
practicalswift:const-ref-parameters

practicalswift commented Dec 6, 2020

Uh oh!

DrahtBot commented Dec 6, 2020

Uh oh!

DrahtBot commented Dec 6, 2020

Uh oh!

maflcko commented Dec 6, 2020

Uh oh!

hebasto left a comment

Uh oh!

practicalswift commented Dec 6, 2020

Uh oh!

fjahr commented Dec 6, 2020

Uh oh!

luke-jr left a comment

Uh oh!

glozow Jan 18, 2021

Uh oh!

maflcko Jan 19, 2021

Uh oh!

practicalswift Jan 19, 2021 •

edited

Loading

Uh oh!

glozow Jan 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Conversation

practicalswift commented Dec 6, 2020

Uh oh!

DrahtBot commented Dec 6, 2020

Conflicts

Uh oh!

DrahtBot commented Dec 6, 2020

Uh oh!

maflcko commented Dec 6, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Dec 6, 2020

Uh oh!

fjahr commented Dec 6, 2020

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

glozow Jan 18, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 19, 2021

Choose a reason for hiding this comment

Uh oh!

practicalswift Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glozow Jan 19, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

practicalswift Jan 19, 2021 •

edited

Loading