Many symbols in the files were undefined and causing issues when I was working on building independent sections of the codebase. The hidden imports from the "secp256k1" library was a particular pain point.
The other standard and missing includes are following best practices and will help with refactoring, build process and others.
Clean up and declared imports/include for pubkey.cpp and pubkey.h
Would like feedback on the comments next to the imports.
sometimes they can be helpful, but assuming one imports multiple things I’m not sure what styling to take or just omit.
17 | +#include <uint256.h> // for u256 18 | 19 | -#include <stdexcept> 20 | -#include <vector> 21 | +#include <cstring> // for memcmp, memcpy, size_t 22 | +#include <vector> // for vector
Would like feedback on the comments next to the imports.
That is not something we want to do. What is the value-add of putting // for vector next to #include <vector>? In the other cases, these comments would be fairly unmaintainable, frequently become out of date (if not managed in some automated way), would pollute diffs etc.
I agree. Changes have been implemented by removing comments from includes.
I'd much rather we went the other way and removed all of the // for comments from include lines, for the reasons of maintainability explained here: #21745 (review).
I'd much rather we went the other way and removed all of the
// forcomments from include lines, for the reasons of maintainability explained here: #21745 (comment).
Agreed all around then. I've updated the code and removed the "for" comments next to include. Better to include those comments in the PR itself.
Should be good to go now.
13 | +#include <span.h> 14 | +#include <uint256.h> 15 | + 16 | +#include <algorithm> 17 | + 18 | +#include <assert.h>
#include <cassert>
#include <algorithm>
(group all standard library imports together and prefer the c++ versions of the c headers).
Done. Yep that was a question I was slightly mulling over. I'll take a look and see if i see the styling docs (if any).
Looks better. All commits can be squashed.
removed comments next to include statements.
removed comments in include statements.
changed assert.h to cassert based on pr comments
Looks better. All commits can be squashed.
Commits got squashed. let me know if you need anything else
utACK 71c824ed6c
Change looks good. Did you use a tool to find the missing includes?
Change looks good. Did you use a tool to find the missing includes?
I am working on currently writing up a CMake build for the repo and did run into several issues especially with the secp256k1 libraries due to some different includes within that lib/module.
I generally just crank up all the warnings and flags for the compilers I use and other static analyzers, which also bring in "include what you use" tool.
Code review ACK 71c824ed6cf70b39ca09e8b3962f452f69523af0 Checked that nothing outrageous was added, I did not verify that all the added includes are necessary individually.