Conversation
|
utACK |
|
utACK 073613b |
jonasschnelli
left a comment
jonasschnelli
left a comment
There was a problem hiding this comment.
utACK 073613b36a8780dd720e2a17f1314980173e200d
|
utACK 073613b |
|
Good idea, rpcserver is supposed to be bitcoin-agnostic, those methods don't belong there. |
|
There's also GetDifficulty (declared in rpc/server.h, defined in rpc/blockchain.cpp, used in several rpc places) which isn't taken care of here. I wasn't sure what to do with it. |
|
utACK 073613b36a8780dd720e2a17f1314980173e200d |
1 similar comment
|
utACK 073613b36a8780dd720e2a17f1314980173e200d |
jnewbery
left a comment
jnewbery
left a comment
There was a problem hiding this comment.
tested ACK. Looks good.
There's also GetDifficulty (declared in rpc/server.h, defined in rpc/blockchain.cpp, used in several rpc places) which isn't taken care of here
Could that become a member function of CBlockIndex in chain.cpp?
src/rpc/server.h
Outdated
There was a problem hiding this comment.
Can you also move nWalletUnlockTime to rpcwallet.h as an extern declaration? You'll need to #include wallet/rpcwallet.h in rpc/misc.cpp, protected by #ifdef ENABLE_WALLET
|
Needs rebase (and last nit-fix) |
jtimon
force-pushed
the
0.15-extern-rpc-server
branch
from
073613b to
24b2c40
Compare
|
Rebased. |
|
removing the unnecessary 'extern' in functions
I think there are other functions with 'extern'. You might as well do it
for the whole code base.
…On Mon, Mar 6, 2017 at 10:59 PM, Jorge Timón ***@***.***> wrote:
Rebased.
Rebasing makes the first commit unnecessary after #8775
<#8775> "a4356328 Move wallet RPC
declarations to rpcwallet.h" . It seems nWalletUnlockTime was moved in
#8775 <#8775> too (see "2e518e31
Move nWalletUnlockTime to CWallet::nRelockTime, and name timed task unique
per CWallet" ).
This leaves the PR to a single commit removing the unnecessary 'extern' in
functions, trivial to review.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#9845 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AGGmv_Wl53kJR1JBq0nGpahZfM9Q7dgzks5rjIG1gaJpZM4MKol3>
.
|
|
@MarcoFalke I guess at that point I should create another PR since the tittle "RPC: cleanups in rpc/server" wouldn't fit anymore. I'm not planning to do that in the short term, but you are right. I would happily ACK such a PR to replace this one if anybody does it. I was just working on rpcwallet and rpc/server and saw easy things to fix without knowing about #8775 . I may end up doing it myself if nobody else jumps at it, but for now I'll just leave this PR open as it is. |
For whoever does that: do add instruction in |
|
|
||
| CAmount AmountFromValue(const UniValue& value); | ||
| UniValue ValueFromAmount(const CAmount& amount); | ||
| double GetDifficulty(const CBlockIndex* blockindex = NULL); |
There was a problem hiding this comment.
Why is GetDifficulty still here? :-)
|
extern should never be used for functions, but I'm not sure that is worth to add "please don't add extern to functions because it doesn't do anything" to the developer notes, at least not more than "please don't add variables that aren't used". Should we merge this anyway (since it's trivial, has been reviewed and it's not disruptive) or should I just close it? |
|
Closing, it seems at this point would be preferable to remove extern from all functions from the project at once (I bet they're not that many, but I didn't looked yet) as @MarcoFalke suggests. |
|
I think removing GetDifficulty from server.h would have made a lot of sense here, along with the other cleanups as those lines were touched anyway, but indeed it's very low priority. |
|
Well, as said I wasn't sure what to do with GetDifficulty, so this PR never moved it. |
|
No need to be clever here, if it's defined in |
11 participants
There's no reason to declare the following functions in rpc/server.h when they're defined in wallet/rpcwallet.cpp. They can be declared in wallet/rpcwallet.h instead:
HelpRequiringPassphrase
EnsureWalletIsAvailable
EnsureWalletIsUnlocked
Also, there's no reason to use extern for functions declared in rpc/server.h. It is really never needed (see http://stackoverflow.com/questions/11712707/extern-functions-in-c-vs-c/11712820#11712820 ), but it seems more strange when those functions are also defined in rpc/server.cpp (in the same module).