Page MenuHomePhabricator

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

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

Details

Reviewers
schancel
jasonbcox
deadalnix
Group Reviewers
Restricted Project
Commits
rABC4967c68c3891: [rpc] Move tojson.h into blockchain.h
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

markblundeberg created this revision.May 10 2019, 05:13
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 10 2019, 05:13

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
schancel added inline comments.May 12 2019, 23:48
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.

schancel accepted this revision.May 13 2019, 18:45
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.

This revision was automatically updated to reflect the committed changes.