refactor: Use uint16_t instead of unsigned short

ahook commented at 10:45 pm on December 28, 2019: none

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.

fanquake added the label Refactoring on Dec 28, 2019

fanquake commented at 10:54 pm on December 28, 2019: member

Each use of unsigned short is for a port value.

The usage in src/secp256k1/src/tests.c is not for a port value, nor in src/addrdb.cpp. Also, modifications to subtrees need to be sent upstream.

Previous similar refactors: #8394, #15586.

ahook commented at 11:08 pm on December 28, 2019: none

Updated the comment in the description and reverted the secp256k1 change.

Also, props for finding those other related PRs so fast!

practicalswift commented at 0:13 am on December 29, 2019: contributor

Concept ACK: explicit is better - let’s get rid of unsigned short once and for all :)

This PR is trivially correct (s/unsigned short/uint16_t/ in this PR combined with the existing assumption static_assert(sizeof(short) == 2, "16-bit short assumed"); in src/compat/assumptions.h) in contrast to the somewhat related PRs linked by @fanquake above.

DrahtBot commented at 4:50 am on December 29, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17877 (qt, refactor: Make enums in BitcoinUnits class scoped by hebasto)
#17453 (gui: Fix intro dialog labels when the prune button is toggled by hebasto)
#17428 (p2p: Try to preserve outbound block-relay-only connections during restart by hebasto)
#15112 (build: Optionally enable -Wzero-as-null-pointer-constant by Empact)

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.

promag commented at 3:35 pm on December 29, 2019: member

ACK, needs squash.

fanquake requested review from dongcarl on Dec 29, 2019

in src/addrdb.cpp:13 in b6ca712a9b outdated

 9@@ -10,6 +10,7 @@
10 #include <clientversion.h>
11 #include <hash.h>
12 #include <random.h>
13+#include <stdint.h>

practicalswift commented at 4:31 pm on December 29, 2019:

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.

ahook commented at 5:21 pm on December 29, 2019:

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.

ahook commented at 8:35 pm on December 29, 2019:

Ah neat, I was mistaken, cstdint provides the global-namespace types as well as the std namespace. Just swapping the includes was sufficient.

elichai commented at 10:07 am on January 19, 2020:

@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#pullrequestreview-344990693

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)

in src/serialize.h:255 in b6ca712a9b outdated

251@@ -252,7 +252,7 @@ template<typename Stream> inline void Unserialize(Stream& s, bool& a) { char f=s
252 inline unsigned int GetSizeOfCompactSize(uint64_t nSize)
253 {
254     if (nSize < 253)             return sizeof(unsigned char);
255-    else if (nSize <= std::numeric_limits<unsigned short>::max()) return sizeof(unsigned char) + sizeof(unsigned short);
256+    else if (nSize <= std::numeric_limits<uint16_t>::max()) return sizeof(unsigned char) + sizeof(uint16_t);

hebasto commented at 4:59 pm on December 29, 2019:

s/std::numeric_limits<uint16_t>::max()/UINT16_MAX/ Ref: https://en.cppreference.com/w/cpp/types/integer

nm

practicalswift commented at 5:12 pm on December 29, 2019:

Why? :) numeric_limits is the preferred choice in C++11 code, no?

hebasto commented at 5:15 pm on December 29, 2019:

@practicalswift agree.

in src/serialize.h:269 in b6ca712a9b outdated

265@@ -266,7 +266,7 @@ void WriteCompactSize(Stream& os, uint64_t nSize)
266     {
267         ser_writedata8(os, nSize);
268     }
269-    else if (nSize <= std::numeric_limits<unsigned short>::max())
270+    else if (nSize <= std::numeric_limits<uint16_t>::max())

hebasto commented at 5:00 pm on December 29, 2019:

s/std::numeric_limits<uint16_t>::max()/UINT16_MAX/ Ref: https://en.cppreference.com/w/cpp/types/integer

nm (https://github.com/bitcoin/bitcoin/pull/17822#discussion_r361862264)

hebasto changes_requested

hebasto commented at 5:01 pm on December 29, 2019: member

Concept ACK.

practicalswift commented at 8:51 pm on December 29, 2019: contributor

ACK ee71d9584d94c774cdba029f986b1cecd3774b61 modulo squash of the three commits into one :)

refactor: Use uint16_t instead of unsigned short c3e3700540

ahook force-pushed on Dec 29, 2019

practicalswift commented at 8:06 pm on January 12, 2020: contributor

ACK c3e3700540d8e2f5b2738b6d0a4e882db9d450a2

elichai changes_requested

elichai commented at 10:00 am on January 19, 2020: contributor

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

hebasto commented at 10:03 am on January 19, 2020: member

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 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

#17822 (review)

DrahtBot commented at 5:36 pm on January 27, 2020: member

DrahtBot added the label Needs rebase on Jan 27, 2020

jonatack commented at 5:47 pm on March 16, 2020: member

ACK. @ahook this PR has several ACKS and only needs a rebase; can you revisit?.

fanquake added the label Up for grabs on Apr 26, 2020

fanquake closed this on Apr 26, 2020

fanquake removed the label Needs rebase on Apr 26, 2020

MarcoFalke removed the label Up for grabs on Jun 17, 2020

laanwj referenced this in commit 0d69fdb9a0 on Jul 9, 2020

sidhujag referenced this in commit 4454ed1cb1 on Jul 9, 2020

DrahtBot locked this on Feb 15, 2022

refactor: Use uint16_t instead of unsigned short #17822

Conflicts