Avoid usage of uninitialized values in function call arguments.
Rationale:
Avoid used-before-set errors and their associated undefined behavior. Avoid problems with comprehension of complex initialization. Simplify refactoring.
For a more thorough discussion, see "ES.20: Always initialize an object" in the C++ Core Guidelines (Stroustrup & Sutter).
12 | @@ -13,7 +13,7 @@ 13 | 14 | static void Base58Encode(benchmark::State& state) 15 | { 16 | - unsigned char buff[32] = {
Why?
@sipa To get rid of clang-tidy, Clang Static Analyzer and other static analyser warnings along the lines of:
bench/base58.cpp:23:9: warning: Function call argument is a pointer to uninitialized value [clang-analyzer-core.CallAndMessage]
That seems overly pedantic...
Is that because b+32 is passed to the EncodeBase58 function and clang thinks the function might use it?
It's perfectly legal to use a pointer that points one past the end of an object (and the function does). You can't dereference it, though.
Yeah but is that the only reason clang is giving that warning? I agree that it would be pedantic to add an extra element to the array just to get rid of that warning, seems very hacky
If we're going to change this for pedantic correctness (which I'm not arguing for), I'd 100x rather make this a std::array<unsigned char, 32> and iterate properly.
Now using std::array<unsigned char, 32>. Both clang-tidy and Clang Static Analyzer are now happy :-)
"ES.20: Always initialize an object"
I disagree. Adding superfluous initialization can conceal real bugs when it prevents valgrind and MSAN from seeing code accessing uninitialized memory because it's been filled with dummy data. This isn't a hypothetical concern-- I've both seen errors caught this way, and real errors hidden by excess initialization.
If the initialization the appropriate initial state, then by all means... but if any use of the initialized value would be a program error then it probably should not be initialized.
Rationale:
> Avoid used-before-set errors and their associated undefined behavior. Avoid problems with comprehension of complex initialization. Simplify refactoring.
For a more thorough discussion, see ["ES.20: Always initialize an object"](https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es20-always-initialize-an-object) in the C++ Core Guidelines (Stroustrup & Sutter).
@gmaxwell You have a good point there with regards to discovering issues via valgrind and MSAN. (Perhaps worth adding as an counter-argument/exception to ES.20 in the C++ Core Guidelines?)
In this specific case with these six changes - are there such risks associated with any of the changes introduced here? Let me know if so and I'll skip any such change.
From what I can see, the specific changes here should be fine, I believe it was more of a general note (which I do agree with).
In this specific case with these six changes - are there such risks associated with any of the changes introduced here? Let me know if so and I'll skip any such change.
There are always risks associated with changes. Especially in consensus-critical code we should thus not make unnecessary ones.
@laanwj Is there any of these six changes that touches on consensus-critical code and should be considered unnecessary? Let me know and I'll exclude from this PR :-)
I think none of the changes here on themselves are risky, but all of them hide potential future bugs.
Thanks for reviewing and thanks for some insightful comments with regards to early initialization potentially hiding issues from runtime detection of code accessing uninitialized memory (when using say valgrind or MSAN).
The consensus is a clear NACK with regards to early initialization, so I'll close this PR.
I'll create a separate PR with with the non-initialization change: switching from char[32] to std::array<unsigned char, 32> to avoid warnings from various static analyzers (clang-tidy, clang analyzer, etc.) regarding pointers to uninitialized values as function call arguments when we use f(arr, arr + 32).