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

pull practicalswift wants to merge 2 commits into bitcoin:master from practicalswift:const-member-fn-and-const-ref-var changing 18 files +28 −28

practicalswift commented at 7:18 PM on December 6, 2020: contributor
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 check (list of clang-tidy checks)
- cppcheck's constVariable check (list of cppcheck checks)
See #18920 for instructions on how to analyse Bitcoin Core using Clang Static Analysis, clang-tidy and cppcheck.
Don't declare de facto const member functions as non-const 1c65c075ee
Don't declare de facto const reference variables as non-const 31b136e580
DrahtBot added the label Consensus on Dec 6, 2020
DrahtBot added the label Mining on Dec 6, 2020
DrahtBot added the label P2P on Dec 6, 2020
DrahtBot added the label RPC/REST/ZMQ on Dec 6, 2020
DrahtBot added the label Validation on Dec 6, 2020
DrahtBot added the label Wallet on Dec 6, 2020
MarcoFalke removed the label Consensus on Dec 7, 2020
MarcoFalke removed the label Mining on Dec 7, 2020
MarcoFalke removed the label P2P on Dec 7, 2020
MarcoFalke removed the label RPC/REST/ZMQ on Dec 7, 2020
MarcoFalke removed the label Validation on Dec 7, 2020
MarcoFalke removed the label Wallet on Dec 7, 2020
MarcoFalke added the label Refactoring on Dec 7, 2020
in src/net.cpp:1219 in 31b136e580
```
1215 | @@ -1216,7 +1216,7 @@ void CConnman::NotifyNumConnectionsChanged()
1216 |      }
1217 |  }
1218 |  
1219 | -void CConnman::InactivityCheck(CNode *pnode)
1220 | +void CConnman::InactivityCheck(CNode *pnode) const
```
jonatack commented at 11:53 AM on December 7, 2020:

1c65c075 not saying it's wrong, but this change to const member function for net.{h,cpp}::CConnman::InactivityCheck, in which pnode->fDisconnect can be mutated, I find somewhat odd in terms of communicated intent

practicalswift commented at 10:04 AM on December 8, 2020:

Given c of type CConnman do we agree that the observable state of c is unaffected by c.InactivityCheck(p)? :)

That p can be mutated by c.InactivityCheck(p) shouldn't be odd since that follows from the signature, no? :)
jonatack commented at 11:56 AM on December 7, 2020: contributor

ACK 31b136e5802e1b1e5f9a9589736afe0652f34da2
DrahtBot commented at 4:09 PM on December 9, 2020: contributor



🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.
DrahtBot cross-referenced this on Dec 17, 2020 from issue Add I2P support using I2P SAM by vasild
DrahtBot commented at 9:02 PM on December 17, 2020: contributor


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #20864 (net: Move SocketSendData lock annotation to header by MarcoFalke)
- #20833 (rpc/validation: enable packages through testmempoolaccept by glozow)
- #20750 ([Bundle 2/n] Prune g_chainman usage in mempool-related validation functions by dongcarl)
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 cross-referenced this on Dec 19, 2020 from issue tree-wide: De-globalize ChainstateManager by dongcarl
DrahtBot cross-referenced this on Dec 22, 2020 from issue [Bundle 2/n] Prune g_chainman usage in mempool-related validation functions by dongcarl
theStack approved
theStack commented at 12:54 PM on December 28, 2020: contributor

ACK 31b136e5802e1b1e5f9a9589736afe0652f34da2 :snowflake:
DrahtBot cross-referenced this on Jan 3, 2021 from issue rpc/validation: enable packages through testmempoolaccept by glozow
DrahtBot cross-referenced this on Jan 6, 2021 from issue net: Move SocketSendData lock annotation to header by MarcoFalke
ajtowns commented at 1:35 AM on January 7, 2021: contributor

ACK 31b136e5802e1b1e5f9a9589736afe0652f34da2

Don't double negatives? ("Declare de facto const reference variables/member functions as const" -- might be worth updating the PR title at least if you don't need to invalidate acks for some other reason)
MarcoFalke renamed this:
~~Don't declare de facto const member functions as non-const. Don't declare de facto const reference variables as non-const.~~
Declare de facto const reference variables/member functions as const
on Jan 7, 2021
MarcoFalke merged this on Jan 7, 2021
MarcoFalke closed this on Jan 7, 2021
sidhujag referenced this in commit bdac8f2acb on Jan 7, 2021
practicalswift deleted the branch on Apr 10, 2021
Fabcien referenced this in commit 6576374b2a on Feb 24, 2022
bitcoin locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#18920 doc: Document how to analyze Bitcoin Core using Clang Static Analysis, clang-tidy and cppcheck #20581 Don't make "in" parameters look like "out"/"in-out" parameters: pass by ref to const instead of ref to non-const

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

Conflicts