refactor: Use uint16_t instead of unsigned short #17822
Conversation
The usage in |
|
Updated the comment in the description and reverted the secp256k1 change. Also, props for finding those other related PRs so fast! |
|
Concept ACK: explicit is better - let's get rid of This PR is trivially correct ( |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
|
ACK, needs squash. |
src/addrdb.cpp
Outdated
| @@ -10,6 +10,7 @@ | |||
| #include <clientversion.h> | |||
| #include <hash.h> | |||
| #include <random.h> | |||
| #include <stdint.h> | |||
There was a problem hiding this comment.
Nit: Use #include <cstdint> throughout this PR. The #include <stdint.h> form for including C standard library headers is deprecated (but unlikely to be purged any time soon) :)
Generally #include <cfoo> is preferred over #include <foo.h> (deprecated) where foo.h is a C standard library header.
There was a problem hiding this comment.
Sounds good, will make that change. I intentionally used the deprecated version simply because there were 138 instances of stdint.h and only 18 cstdint, but I agree cstdint is the better option. Only downside of cstdint is that we have to tack on std:: to all the uses.
There was a problem hiding this comment.
Ah neat, I was mistaken, cstdint provides the global-namespace types as well as the std namespace. Just swapping the includes was sufficient.
There was a problem hiding this comment.
@practicalswift I disagree.
If we want to switch to the non deprecated <cstdint> we will need to add something like using std::uint16_t as this header doesn't promise injecting the types into global namespace and FWIW the support for C headers is still in C++17 standard and the current draft of C++20 .
See #17822 (review)
Using <cstdint> and assuming types are in global namespace is the worst option IMHO, it means we prefer using non-standard implementation defined detail just to not use something the standard explicitly supports but says might be removed in the future (aka deprecated) especially when it seems the earliest it might be removed is in C++23 when A. we are still using C++11. B. I don't believe it will be removed then. (although my beliefs are irrelevant)
| @@ -266,7 +266,7 @@ void WriteCompactSize(Stream& os, uint64_t nSize) | |||
| { | |||
| ser_writedata8(os, nSize); | |||
| } | |||
| else if (nSize <= std::numeric_limits<unsigned short>::max()) | |||
| else if (nSize <= std::numeric_limits<uint16_t>::max()) | |||
There was a problem hiding this comment.
s/std::numeric_limits<uint16_t>::max()/UINT16_MAX/
Ref: https://en.cppreference.com/w/cpp/types/integer
nm (#17822 (comment))
|
ACK ee71d95 modulo squash of the three commits into one :) |
|
ACK c3e3700 |
There was a problem hiding this comment.
Please replace cstdint with stdint.h
cstdint doesn't require putting the types in global namespace, meaning if you use that header you need to do std::uint16_t.
Only stdint.h promise that the int types will be in global namespace, and these headers are part of C++ standard (see D.5)
D.5.3: (latest draft I could find of C++11: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3242.pdf )
[Example: The header <cstdlib> assuredly provides its declarations and definitions within the namespace std. It may also provide these names within the global namespace. The header <stdlib.h> assuredly provides the same declarations and definitions within the global namespace, much as in the C Standard. Itmay also provide these names within the namespacestd.— end example
|
| Needs rebase |
|
ACK. @ahook this PR has several ACKS and only needs a rebase; can you revisit?. |
1cabbdd refactor: Use uint16_t instead of unsigned short (Aaron Hook) Pull request description: 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 (: ACKs for top commit: sipsorcery: ACK 1cabbdd. practicalswift: ACK 1cabbdd -- patch looks correct :) laanwj: ACK 1cabbdd hebasto: ACK 1cabbdd, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 0e6bf64f274aae5dacb188358b4d5f65ccb207d4f70922f039bc4ed7934709418ddad19f8bfb7462517427837c3d2bb3f86ef284bb40e87119aad2a1e148d9d6
1cabbdd refactor: Use uint16_t instead of unsigned short (Aaron Hook) Pull request description: I wanted to see if the `up for grabs` label works and looked at PR bitcoin#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 (: ACKs for top commit: sipsorcery: ACK 1cabbdd. practicalswift: ACK 1cabbdd -- patch looks correct :) laanwj: ACK 1cabbdd hebasto: ACK 1cabbdd, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 0e6bf64f274aae5dacb188358b4d5f65ccb207d4f70922f039bc4ed7934709418ddad19f8bfb7462517427837c3d2bb3f86ef284bb40e87119aad2a1e148d9d6
The uint16_t type (defined in stdint.h and cstdint) is used all over the codebase. A git grep shows 149 instances of uint16_t and only 21 instances of unsigned short. Most uses of unsigned short in the codebase are for port values. Besides being a more compact/readable typename, uint16_t is also a more precise representation of a port than unsigned short: uint16_t is explicitly 16 bits while unsigned short has a a weaker specification of >= 16 bits.