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!
utACK 64aa36e
utACK
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.
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.
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.
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.
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.
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.
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.