Page MenuHomePhabricator

[rpc] Move tojson.h into blockchain.h
ClosedPublic

Authored by markblundeberg on May 10 2019, 05:13.

Details

Summary

In order to make backports go smoothly. (file was introduced in D717)

Test Plan

compile

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

note: TxToJSON is not in blockchain.cpp and doesn't need to be in a header either

schancel requested changes to this revision.May 12 2019, 23:47

I think these should be left in place.

This revision now requires changes to proceed.May 12 2019, 23:47
src/rpc/tojson.h
14 ↗(On Diff #8582)

If you're going to delete this from the header, you should mark its implementation as static in rawtransaction.cpp

I think these should be left in place.

Why?

I think more and more stuff should be better organized. Core doesn't seem to care about separating stuff out much.

However, this is just my opinion. It seems Amaury shared it. I hope we don't prevent ourself from making needed organizational changes indefinitely because of backports.

I think more and more stuff should be better organized. Core doesn't seem to care about separating stuff out much.

However, this is just my opinion. It seems Amaury shared it. I hope we don't prevent ourself from making needed organizational changes indefinitely because of backports.

Ah I see. Well the original organization was definitely crap, as both Core and Amaury recognized. I guess my mental picture here is that if the backport of PR10095 had already been done, then the tojson.h file would never have been made in the first place and a divergence would not have occurred. I also personally think the blockchain.h is the right place for these functions (being defined in blockchain.cpp).

Okay, I see. That's weird -- still not sure *toJSON functions should go there, but okay.

This revision is now accepted and ready to land.May 13 2019, 18:45

Okay, I see. That's weird -- still not sure *toJSON functions should go there, but okay.

Yeah, as I was typing that I was thinking "why are they in blockchain.cpp anyway?".. Well, once these functions are caught up we can look at messing around with them.