I wanted to see if the up for grabs label works and looked at PR #17822 originally opend by ahook I saw it had many acks for example by jonatack and practicalswift but needed rebasing.
So I checked out the remote branch rebased it resolved three conflicts and continued the rebase.
Hope everything is as expected (:
Concept ACK: explicit is better than implicit.
Warm welcome as a contributor @renepickhardt - I’m a big fan of your Lightning work etc :) Looking forward to reading Mastering the Lightning Network once it is released!
FWIW, we are already assuming that sizeof(uint16_t) == sizeof(unsigned short) implicitly in multiple places like here …
… and also explicitly in assumptions.h …
btw running git grep "unsigned short" results in:
0src/secp256k1/src/tests.c: unsigned short sh;
I could make another commit to this branch where I also changed that but I am not sure if the bitcoin repo is the place to patch libsecp256k1 or if that should be done in https://github.com/bitcoin-core/secp256k1
at least the remote file https://github.com/bitcoin-core/secp256k1/blob/master/src/tests.c has 19 contributers according to git blame where in this repo it only has contributes from @sipa and @laanwj
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
5@@ -6,6 +6,7 @@
6 #define BITCOIN_QT_OPTIONSMODEL_H
7
8 #include <amount.h>
9+#include <cstdint>
removed trailing whitespace to make linter happy
Ok so I have squashed the two commits and rebased to current master to make merging easier (:
if there is anything more to do let me know @practicalswift
ACK 1cabbddbca615b26aa4510c75f459c28d6fe0afd.
Nice to replace a C header with a C++ header as well.
ACK 1cabbddbca615b26aa4510c75f459c28d6fe0afd – patch looks correct :)
If anyone wants to follow up by checking potentially unintended/unwarranted uses of long this command might be helpful to find candidates :)
0$ git grep -E '[^a-zA-Z_."]long[^a-zA-Z_."]' -- "*.cpp" "*.h" ":(exclude)src/univalue/" ":(exclude)src/qt/" ":(exclude)src/test/" ":(exclude)src/crc32c/" ":(exclude)src/crypto/common.h" ":(exclude)src/randomenv.cpp" | grep -vE '^[^ ]*:.*[/*]' | grep -vE "(too.long|ftell|strtoul|strol|strtol|how long|really long|fseek|as long)"
1src/httpserver.cpp: int workQueueDepth = std::max((long)gArgs.GetArg("-rpcworkqueue", DEFAULT_HTTP_WORKQUEUE), 1L);
2src/httpserver.cpp: int rpcThreads = std::max((long)gArgs.GetArg("-rpcthreads", DEFAULT_HTTP_THREADS), 1L);
3src/rest.cpp: if (headers.size() == (unsigned long)count)
4src/streams.h: long nLongPos = nPos;
5src/txmempool.cpp: if (coin.IsSpent() || (coin.IsCoinBase() && ((signed long)nMemPoolHeight) - coin.nHeight < COINBASE_MATURITY)) {
6src/txmempool.h: unsigned long size() const
unsigned short does in fact need to be an unsigned short…
36@@ -36,7 +37,7 @@ template <typename Data>
37 bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data& data)
38 {
39 // Generate random temporary filename
40- unsigned short randv = 0;
41+ uint16_t randv = 0;
42 GetRandBytes((unsigned char*)&randv, sizeof(randv));
43 std::string tmpfn = strprintf("%s.%04x", prefix, randv);
strprintf has some type magic.
strprintf is type safe. There is never a need for size specifiers.
110@@ -110,9 +111,9 @@ void CConnman::AddOneShot(const std::string& strDest)
111 vOneShots.push_back(strDest);
112 }
113
114-unsigned short GetListenPort()
115+uint16_t GetListenPort()
116 {
117- return (unsigned short)(gArgs.GetArg("-port", Params().GetDefaultPort()));
118+ return (uint16_t)(gArgs.GetArg("-port", Params().GetDefaultPort()));
The point is to make a potentially sign-changing narrowing conversion explicit.
GetArg returns int64_t so it makes sense to make the implicit conversion to uint16_t explicit using a cast.
798@@ -798,11 +799,11 @@ bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDest, int
799 ProxyCredentials random_auth;
800 static std::atomic_int counter(0);
801 random_auth.username = random_auth.password = strprintf("%i", counter++);
802- if (!Socks5(strDest, (unsigned short)port, &random_auth, hSocket)) {
803+ if (!Socks5(strDest, (uint16_t)port, &random_auth, hSocket)) {
port?
ACK 1cabbddbca615b26aa4510c75f459c28d6fe0afd
After this, the only mention of unsigned short left is in the secp256k1 tests, which should not be changed here
In addition to @luke-jr’s comment, could the type of the port parameter in the Socks5() function be changed from int to uint16_t?
I understand why @renepickhardt didn’t do this. Right now, this is a pure refactor, only changing a type to a typedef of the same type (for, in practice, all architectures supported by bitcoin). Changing the parameter type would be an actual code change.
In addition to @luke-jr’s comment, could the type of the port parameter in the Socks5() function be changed from int to uint16_t?
I understand why @renepickhardt didn’t do this. Right now, this is a pure refactor, only changing a type to a typedef of the same type (for, in practice, all architectures supported by bitcoin). Changing the parameter type would be an actual code change.
Agree.
ACK 1cabbddbca615b26aa4510c75f459c28d6fe0afd, I have reviewed the code and it looks OK, I agree it can be merged.