switch from boost int types to <stdint.h> #4129

kdomanski commented at 6:17 PM on May 5, 2014: contributor

According to Jeff in #4025 (comment): "boost::int64_t deserves a quick and violent death. All our platforms can handle stdint.h types such as int64_t or uint64_t".

sipa commented at 6:25 PM on May 5, 2014: member

Are the changes to json_spirit necessary? They'll potentially conflicts with changes pulled in from upstream.

laanwj commented at 6:49 PM on May 5, 2014: member

@sipa I'm afraid they are. json_spirit is the worst offender in using boost::[u]int64_t, I think it's even the source of all of those in the bitcoin source.

sipa commented at 6:50 PM on May 5, 2014: member

Has anyone tried just using int64_t in our code, and not touching those in json_spirit? Whatever they're typedef'd to, they should be convertible in both directions.

I don't care what json_spirit is doing.

sipa commented at 6:51 PM on May 5, 2014: member

Alternatively, we can define our own json_spirit configuration type I think, which uses different types than what their code uses by default.

EDIT: nevermind, it seems to be harder than that.

laanwj commented at 6:51 PM on May 5, 2014: member

Yes, that has been tried, and it doesn't work. The reason all those casts are in RPC code is not for nothing. One of them defines the int64_t as long, the other as long long. Using them interchangeably gives errors on some architectures.

sipa commented at 6:56 PM on May 5, 2014: member

Ok, sorry - I really hoped to avoiding needing to touch the json_spirit code.

laanwj commented at 7:02 PM on May 5, 2014: member

I agree it would be preferable!

Though - the last time it has been tried to upgrade json_spirit from upstream it failed and we had to revert, due to rounding-related issues (see #3127, and my comment in #751). So it would have to be done very carefully already (...at least as long as we still use the json numeric for monetary amounts).

kdomanski commented at 7:39 PM on May 5, 2014: contributor

Huh, it compiled fine for me. Anyways, this next commit adds missing stdint.h.

However, I wonder if <cinttypes> wouldn't be just as good.

in src/rpcwallet.cpp:None in 91a67cf61b outdated

  56 | @@ -57,8 +57,8 @@ void WalletTxToJSON(const CWalletTx& wtx, Object& entry)
  57 |      BOOST_FOREACH(const uint256& conflict, wtx.GetConflicts())
  58 |          conflicts.push_back(conflict.GetHex());
  59 |      entry.push_back(Pair("walletconflicts", conflicts));
  60 | -    entry.push_back(Pair("time", (boost::int64_t)wtx.GetTxTime()));
  61 | -    entry.push_back(Pair("timereceived", (boost::int64_t)wtx.nTimeReceived));
  62 | +    entry.push_back(Pair("time", (int64_t)wtx.GetTxTime()));

laanwj commented at 9:20 AM on May 6, 2014:

A lot of these casts will not be needed anymore. They were added to quell ambiguity errors between boost::int64_t and system int64_t.

laanwj added this to the milestone 0.10.0 on May 6, 2014

kdomanski commented at 1:10 PM on May 6, 2014: contributor

I removed what I was confident is unnecessary. Waiting for sanity test.

In the meantime, here's a peculiar finding. A lot of remaining casts are there, because some important class keys (for example, in the block header) are declared as either int or unsigned int, whereas protocol specificatioin defines them as either int32_t or uint32_t. Of course, these types are the same on most platforms, but it doesn't make much sense for me to handle them in different places as separate types of the same length and use C-style casts to convert them. I'd like to ammend the code by correcting those types where applicable, both matching the protocol specs more literally and removing some of these casts at the same time.

Opinions?

sipa commented at 1:12 PM on May 6, 2014: member

@kdomanski Fully agree, the fields of protocol classes should have unambiguous length. I think serialize.h should also be converted to only define standard serializations for {int,uint}{8,16,32,64}. But let's do that separately.

laanwj commented at 1:57 PM on May 6, 2014: member

Agreed, although if you change the serialization and protocol stuff we need to be sure that the resulting code is equivalent.

As for this pull I think there's still a fair amount of casts that can go; in general the entry.push_back(Pair("time", (int64_t)acentry.nTime)); don't need a cast as the ambiguity should be gone now? Or am I missing something here?

kdomanski commented at 2:16 PM on May 6, 2014: contributor

This one could go, because CAccountingEntry has unambiguous type for nTime, but I can't see anything more at this time.

in src/rpcnet.cpp:None in 2e73a93975 outdated

 121 | -        obj.push_back(Pair("bytessent", (boost::int64_t)stats.nSendBytes));
 122 | -        obj.push_back(Pair("bytesrecv", (boost::int64_t)stats.nRecvBytes));
 123 | -        obj.push_back(Pair("conntime", (boost::int64_t)stats.nTimeConnected));
 124 | +        obj.push_back(Pair("lastsend", stats.nLastSend));
 125 | +        obj.push_back(Pair("lastrecv", stats.nLastRecv));
 126 | +        obj.push_back(Pair("bytessent", (int64_t)stats.nSendBytes));

laanwj commented at 2:33 PM on May 12, 2014:

These casts also seem unneeded: stats.nSendBytes and nRecvBytes are already uint64_t

laanwj commented at 2:39 PM on May 12, 2014: member

Can you please split the last commit to another pull request? It's unrelated to the pull request title as it doesn't deal with boost types.

in src/rpcblockchain.cpp:None in 2e73a93975 outdated

 321 | -        ret.push_back(Pair("transactions", (boost::int64_t)stats.nTransactions));
 322 | -        ret.push_back(Pair("txouts", (boost::int64_t)stats.nTransactionOutputs));
 323 | -        ret.push_back(Pair("bytes_serialized", (boost::int64_t)stats.nSerializedSize));
 324 | +        ret.push_back(Pair("transactions", (int64_t)stats.nTransactions));
 325 | +        ret.push_back(Pair("txouts", (int64_t)stats.nTransactionOutputs));
 326 | +        ret.push_back(Pair("bytes_serialized", (int64_t)stats.nSerializedSize));

laanwj commented at 2:40 PM on May 12, 2014:

nTransactions ,nTransactionOutputs, nSerializedSize are uint64_t, so no cast needed

BitcoinPullTester commented at 12:30 AM on May 13, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/6a8cb3103a7fb11799ef2711e2efc550c835f994 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

laanwj commented at 5:35 AM on May 13, 2014: member

ACK Looks good to me now.

jgarzik commented at 9:10 AM on May 13, 2014: contributor

ACK

laanwj commented at 9:38 AM on May 13, 2014: member

I would suggest squashing the three 'removed a few unnecessary casts' commits together :)

switch from boost int types to <stdint.h> 4b61a6a478

json_spirit: #include <stdint.h> 3e74ac22d5

removed a few unnecessary casts d56e30ca89

kdomanski commented at 9:42 AM on May 13, 2014: contributor

squashed and rebased against master

laanwj merged this on May 13, 2014

laanwj closed this on May 13, 2014

laanwj referenced this in commit bfae70aae6 on May 13, 2014

kdomanski deleted the branch on May 13, 2014

laanwj referenced this in commit 479179cac7 on May 13, 2014

laanwj referenced this in commit 79144ac17d on May 21, 2014

DrahtBot locked this on Sep 8, 2021