Bloomfilter: parameter variables made constant #9750

rohundhar · 2017-02-13T05:42:17Z

Just a suggestion, since I see none of the variables I've transformed into constants are altered in their respective methods. If changes want to implemented later on so they can be altered, it would be an easy fix but I feel the code is cleaner this way.

let me know your thoughts!

JeremyRubin · 2017-02-13T20:00:15Z

utACK 64aa36e

gmaxwell

utACK

laanwj · 2017-02-14T12:22:17Z

Why would you make non-reference and non-pointer variables const?

Please first add this and the rationale to doc/developer-notes.md. I don't see a point of doing this out of the blue, sorry.

JeremyRubin · 2017-02-14T12:33:50Z

I find it useful when reading code if anything that can be const'd is const'd.

to quote S.O.:

The const qualifier prevents code inside the function from modifying the parameter itself. When a function is larger than trivial size, such an assurance helps you to quickly read and understand a function. If you know that the value of side won't change, then you don't have to worry about keeping track of its value over time as you read. Under some circumstances, this might even help the compiler generate better code.

A non-trivial number of people do this as a matter of course, considering it generally good style.

http://stackoverflow.com/questions/8714250/isnt-const-redundant-when-passing-by-value

There is no harm to making this change, and at best, there is the gain that the above sorts of bug won't later be introduced. Also, in theory such an annotation could help the optimizer better inline such a function.

laanwj · 2017-02-14T13:41:42Z

I agree. However we don't follow everything that is "generally considered good style" by random people on the internet in this project. We most notably don't do this structurally anywhere else, so why start in the bloom filter code specifically?

If you want to introduce this rule, that's fine, I have no reason to be against it, but that needs to be discussed first in a PR adding that recommendation to the developer guide, then merged. Then you can start changing existing code to do that.

jtimon · 2017-02-14T17:09:46Z

utACK 64aa36e . Although as @laanwj points out, we're not doing this anywhere and we shouldn't go crazy trying to change it everywhere at once. If we prefer not to do this for whatever reason, I have no strong opinion.

rohundhar · 2017-02-15T04:55:29Z

Thanks for the discussion. It's true that this standard isn't enforced anywhere else, so i can see how it seems out of the blue. A recent project of mine used bloom filters and so I was interested to check out bitcoin's filter and thought it wouldn't hurt to apply some of the standards i used which I thought were helpful.

I agree, it is kind of random so I'll look more into if I can find a more reasonable argument for the change and then make an attempt on the developer notes.

kallewoof · 2017-02-15T11:18:55Z

I agree that making things const that can be made const is a good idea, taking the virality property as mentioned in Google's C++ styleguide (Use of const) into appropriate consideration.

I also agree that the developer guidelines should include this direction.

laanwj · 2017-02-15T12:37:20Z

One thing I don't like about making by-value parameters const is that something that is private and internal to the function - how their copy of the parameter is handled - needs to be specified on the interface, even though it makes no difference to the ABI. That's a minor problem though and if it's done consistently everywhere I have no problem with it.

Edit: something else that we need to be clarify in the coding style is the use of "override". E.g. I had to add a few in #9764 to prevent gcc from complaining: it doesn't like when it's used inconsistently within a class.

kallewoof · 2017-02-16T02:14:58Z

I didn't even know "override" existed. As for how parameters are handled, I'm of the opinion that function/method parameters should never be changed ever, unless they are explicitly marked as "output parameters", even for primitive types like ints and such, where it doesn't matter.

The primary reason is that it's much easier to read code where the input parameters always stay the same no matter what. I even prefer the following

void foo(const int inX) {
   int x = inX ?: computeStartingX();
   // ...
}

over

void foo(int x) {
   if (!x) x = computeStartingX();
   // ...
}

which is one of the few "ugly" cases with this approach.

64aa36e param variables made const (ロハンダル) Tree-SHA512: 7c19f9e7dd574c8ce8a9468555f27196735b583efe349c1309c90e1e5d2949daf6891574b4bea7122d6c6aca0c7ee4a782fe3d24918d889f7bf89227084a51cd

Summary: 64aa36e param variables made const (ロハンダル) Tree-SHA512: 7c19f9e7dd574c8ce8a9468555f27196735b583efe349c1309c90e1e5d2949daf6891574b4bea7122d6c6aca0c7ee4a782fe3d24918d889f7bf89227084a51cd Backport of Core PR9750 bitcoin/bitcoin#9750 Test Plan: make check Reviewers: deadalnix, Fabien, jasonbcox, markblundeberg, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3406

Summary: 64aa36e param variables made const (ロハンダル) Tree-SHA512: 7c19f9e7dd574c8ce8a9468555f27196735b583efe349c1309c90e1e5d2949daf6891574b4bea7122d6c6aca0c7ee4a782fe3d24918d889f7bf89227084a51cd Backport of Core PR9750 bitcoin/bitcoin#9750 Test Plan: make check Reviewers: deadalnix, Fabien, jasonbcox, markblundeberg, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3406

64aa36e param variables made const (ロハンダル) Tree-SHA512: 7c19f9e7dd574c8ce8a9468555f27196735b583efe349c1309c90e1e5d2949daf6891574b4bea7122d6c6aca0c7ee4a782fe3d24918d889f7bf89227084a51cd

Backport bloom filter improvements Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#7113 - bitcoin/bitcoin#7818 - Only the second commit (to resolve conflicts). - bitcoin/bitcoin#7934 - bitcoin/bitcoin#8655 - Partial backport to help resolve conflicts. - bitcoin/bitcoin#9060 - bitcoin/bitcoin#9223 - bitcoin/bitcoin#9644 - Partial backport to help resolve conflicts. - bitcoin/bitcoin#9916 - bitcoin/bitcoin#9750 - bitcoin/bitcoin#13176 - bitcoin/bitcoin#13948 - bitcoin/bitcoin#16073 - bitcoin/bitcoin#18670 - bitcoin/bitcoin#18806 - Reveals upstream's covert fix for CVE-2013-5700. - bitcoin/bitcoin#19968

param variables made const

64aa36e

gmaxwell approved these changes Feb 13, 2017

View reviewed changes

fanquake added the Refactoring label Feb 14, 2017

merge with bitcoin core

d60d54d

laanwj merged commit d60d54d into bitcoin:master May 18, 2017

dagurval mentioned this pull request Jan 15, 2018

Bloom filter updates bitcoinxt/bitcoinxt#297

Merged

str4d mentioned this pull request Mar 5, 2021

Backport bloom filter improvements zcash/zcash#5026

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Bloomfilter: parameter variables made constant #9750

Bloomfilter: parameter variables made constant #9750

Uh oh!

rohundhar commented Feb 13, 2017 •

edited

Loading

Uh oh!

JeremyRubin commented Feb 13, 2017

Uh oh!

gmaxwell left a comment

Uh oh!

laanwj commented Feb 14, 2017

Uh oh!

JeremyRubin commented Feb 14, 2017

Uh oh!

laanwj commented Feb 14, 2017

Uh oh!

jtimon commented Feb 14, 2017

Uh oh!

rohundhar commented Feb 15, 2017

Uh oh!

kallewoof commented Feb 15, 2017 •

edited

Loading

Uh oh!

laanwj commented Feb 15, 2017 •

edited

Loading

Uh oh!

kallewoof commented Feb 16, 2017

Uh oh!

Uh oh!

Bloomfilter: parameter variables made constant #9750

Bloomfilter: parameter variables made constant #9750

Uh oh!

Conversation

rohundhar commented Feb 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JeremyRubin commented Feb 13, 2017

Uh oh!

gmaxwell left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Feb 14, 2017

Uh oh!

JeremyRubin commented Feb 14, 2017

Uh oh!

laanwj commented Feb 14, 2017

Uh oh!

jtimon commented Feb 14, 2017

Uh oh!

rohundhar commented Feb 15, 2017

Uh oh!

kallewoof commented Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Feb 16, 2017

Uh oh!

Uh oh!

rohundhar commented Feb 13, 2017 •

edited

Loading

kallewoof commented Feb 15, 2017 •

edited

Loading

laanwj commented Feb 15, 2017 •

edited

Loading