Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: Register calls where they are defined #7766

Merged
merged 1 commit into from Mar 31, 2016

Conversation

@laanwj
Copy link
Member

commented Mar 29, 2016

Split out registration of methods to every module, apart from 'help' and 'stop' which are implemented in rpcserver.cpp itself.

  • This makes it easier to add or remove RPC commands - no longer everything that includes rpcserver.h has to be rebuilt when there's a change there.
  • Cleans up rpc/server.h by getting rid of the huge cluttered list of function definitions.
  • Removes most of the bitcoin-specific code from rpcserver.cpp and .h.

Continues #7307 for the non-wallet.

@laanwj laanwj added the RPC/REST/ZMQ label Mar 29, 2016

@laanwj laanwj force-pushed the laanwj:2016_03_separate_rpc_registration branch Mar 29, 2016

@laanwj laanwj added the Refactoring label Mar 29, 2016

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Mar 29, 2016

Nice!
Concept ACK.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Mar 29, 2016

Concept ACK. Would it make sense to get rid of the duplicate code in void Register*()?

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2016

Would it make sense to get rid of the duplicate code in void Register*()?

Maybe. I thought about it, but it's only a simple loop of four lines, sometimes factoring something so simple out to a function makes things harder to understand, not easier.
Another option would be to export a 'descriptor structure' with a name, pointer to the array, and size of the array. E.g. like Linux modules.
But simply having Register* function leaves it up to the module as to how to determine what to register (e.g. there may be optional features enabled on the command line).

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Mar 30, 2016

IMO we should keep the array loop as it is (and not refactor with some hard-to-understand macro-like optimization).

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2016

Could make it two lines, even, without losing any clarity:

   for (unsigned int vcidx = 0; vcidx < ARRAYLEN(commands); vcidx++)
        tableRPC.appendCommand(commands[vcidx].name, &commands[vcidx]);

(will update the one in rpcwallet as well)

@CodeShark

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2016

Concept ACK

@sipa

This comment has been minimized.

Copy link
Member

commented Mar 30, 2016

Concept ACK, though I wonder if it wouldn't be cleaner to pass a pointer to tableRPC to the registration commands, so the individual RPC implementations don't even need to depend on access to the global?

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2016

pass a pointer to tableRPC to the registration commands

Awesome idea, will do that.

@sipa

This comment has been minimized.

Copy link
Member

commented Mar 30, 2016

utACK 3be52e1

1 similar comment
@dcousens

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2016

utACK 3be52e1

rpc: Register calls where they are defined
Split out methods to every module, apart from 'help' and 'stop' which
are implemented in rpcserver.cpp itself.

- This makes it easier to add or remove RPC commands - no longer everything that includes
    rpcserver.h has to be rebuilt when there's a change there.
- Cleans up `rpc/server.h` by getting rid of the huge cluttered list of function definitions.
- Removes most of the bitcoin-specific code from rpcserver.cpp and .h.

Continues #7307 for the non-wallet.
@laanwj

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2016

Squashed a20fca9 0b18725 dcb0a76 3be52e1 into fb8a8cf

@laanwj laanwj force-pushed the laanwj:2016_03_separate_rpc_registration branch to fb8a8cf Mar 31, 2016

@laanwj laanwj merged commit fb8a8cf into bitcoin:master Mar 31, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
laanwj added a commit that referenced this pull request Mar 31, 2016
Merge #7766: rpc: Register calls where they are defined
fb8a8cf rpc: Register calls where they are defined (Wladimir J. van der Laan)
codablock added a commit to codablock/dash that referenced this pull request Dec 19, 2017
Merge bitcoin#7766: rpc: Register calls where they are defined
fb8a8cf rpc: Register calls where they are defined (Wladimir J. van der Laan)
@str4d str4d referenced this pull request Jun 20, 2018
zkbot added a commit to zcash/zcash that referenced this pull request Jun 20, 2018
Auto merge of #3350 - str4d:rpc-reorg, r=<try>
RPC code refactor

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7307
- bitcoin/bitcoin#7348
- bitcoin/bitcoin#7766
zkbot added a commit to zcash/zcash that referenced this pull request Jul 3, 2018
Auto merge of #3350 - str4d:rpc-reorg, r=<try>
RPC code refactor

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7307
- bitcoin/bitcoin#7348
- bitcoin/bitcoin#7766
zkbot added a commit to zcash/zcash that referenced this pull request Jul 16, 2018
Auto merge of #3350 - str4d:rpc-reorg, r=<try>
RPC code refactor

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7307
- bitcoin/bitcoin#7348
- bitcoin/bitcoin#7766
zkbot added a commit to zcash/zcash that referenced this pull request Jul 18, 2018
Auto merge of #3350 - str4d:rpc-reorg, r=<try>
RPC code refactor

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7307
- bitcoin/bitcoin#7348
- bitcoin/bitcoin#7766
zkbot added a commit to zcash/zcash that referenced this pull request Jul 18, 2018
Auto merge of #3350 - str4d:rpc-reorg, r=bitcartel
RPC code refactor

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7307
- bitcoin/bitcoin#7348
- bitcoin/bitcoin#7766
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.