chainparams: Explicitly use uint16 for nDefaultPort #15586
pull dongcarl wants to merge 1 commits into bitcoin:master from dongcarl:2019-03-uint16-port changing 1 files +2 −2-
dongcarl commented at 9:52 pm on March 12, 2019: memberPort numbers are 16-bit unsigned integers.
-
chainparams: Explicitly use uint16 for nDefaultPort
Port numbers are 16-bit unsigned integers.
-
promag commented at 9:55 pm on March 12, 2019: memberCould also update call sites?
-
MarcoFalke commented at 10:03 pm on March 12, 2019: member
Unless you switch the constructors to use member initizlizer lists with the “
{}
-syntax” that checks for overflows, I don’t see how this adds any value.Also, #8394 (comment)
-
fanquake added the label Refactoring on Mar 12, 2019
-
laanwj commented at 10:39 am on March 13, 2019: membersorry, NACK, I don’t see the point, this doesn’t improve the code in any user-visible way
-
ryanofsky approved
-
ryanofsky commented at 2:31 pm on March 13, 2019: memberutACK 7c6b079883b579b1973dd91df7b60bf95e85ef20. Code change itself looks good and is more clear and correct. Will happily defer to Marco/Wladimir on project management & prioritization, though, if this shouldn’t be merged on those grounds.
-
practicalswift approved
-
practicalswift commented at 3:27 pm on March 13, 2019: contributorutACK 7c6b079883b579b1973dd91df7b60bf95e85ef20 (agree with @ryanofsky)
-
dongcarl commented at 2:39 pm on March 14, 2019: member@MarcoFalke You mean use member initializer lists for
CChainParams
? -
dongcarl commented at 2:43 pm on March 14, 2019: memberw/re the parsing comment #8394 (comment), would @laanwj and @MarcoFalke be more inclined to merge if this PR added relevant
ArgsManager
changes? This would be usable for both-port
and-rpcport
I believe. -
dongcarl commented at 3:19 pm on March 14, 2019: member
Just for some context, this was motivated by my looking at
GetListenPort()
, which I believe castssigned int
s tounsigned short
s. I think pairing this change with anArgsManager
change with good parsing would make it worthwhile.Also related: #14856 (review)
-
MarcoFalke commented at 10:04 pm on March 14, 2019: memberI believe it casts
int64_t
tounsigned short
, but that should be fine as long as we check that the value is in range before the cast. No need to change the return type here. -
luke-jr commented at 8:39 pm on April 17, 2019: member
This is just one of many cases where a 16-bit
int
type could cause problems, so I’m okay with fixing it for future portability. But I agree that if we do, we should fix the call sites too:- GetListenPort (in net.cpp)
- CConnman::ConnectNode’s default_port (in net.cpp)
- Lookup’s portDefault, port (in netbase.cpp)
-
- SplitHostPort’s portOut (in util/strencodings.cpp)
- LookupNumeric’s portDefault (in net.cpp)
-
dongcarl commented at 7:42 pm on May 15, 2019: memberI’ll pick this back up someday. Feel free to grab it from me.
-
dongcarl closed this on May 15, 2019
-
dongcarl added the label Up for grabs on May 15, 2019
-
fanquake removed the label Up for grabs on Mar 2, 2021
-
DrahtBot locked this on Aug 16, 2022