wallet: remove unused variable spk

theStack commented at 12:17 pm on November 5, 2019: member

Each of the following RPC functions

importprivkey
importaddress
importpubkey
importwallet
importmulti

define a variable reference LegacyScriptPubKeyMan& spk_man which is not used, leading to compiler warnings in the current master branch (bdda137878904e9401a84e308ac74c93c2ef87c1):

 0  CXX      wallet/libbitcoin_wallet_a-rpcdump.o
 1wallet/rpcdump.cpp:137:28: warning: unused variable 'spk_man' [-Wunused-variable]
 2    LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet);
 3                           ^
 4wallet/rpcdump.cpp:265:28: warning: unused variable 'spk_man' [-Wunused-variable]
 5    LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*pwallet);
 6                           ^
 7wallet/rpcdump.cpp:468:28: warning: unused variable 'spk_man' [-Wunused-variable]
 8    LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet);
 9                           ^
10wallet/rpcdump.cpp:552:28: warning: unused variable 'spk_man' [-Wunused-variable]
11    LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet);
12                           ^
13wallet/rpcdump.cpp:1349:28: warning: unused variable 'spk_man' [-Wunused-variable]
14    LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet);
15                           ^
165 warnings generated.

The call to GetLegacyScriptPubKeyMan() still serves the purpose to throw an error if the wallet doesn’t have a LegacyScriptPubKeyMan instance (indicating that the command is not supported, as pointed out by the error message): https://github.com/bitcoin/bitcoin/blob/bdda137878904e9401a84e308ac74c93c2ef87c1/src/wallet/rpcdump.cpp#L90-L97

This commit ~~casts the functions return value to void to point out that the value is thrown away, and adds a comment on why the function call is needed as well.~~ introduces a new function RequireLegacyScriptPubKeyMan() which in turn calls GetLegacyScriptPubKeyMan() but discards its return value.

fanquake added the label Wallet on Nov 5, 2019

practicalswift commented at 12:21 pm on November 5, 2019: contributor

@theStack Some context is found in #17338 :)

jonatack commented at 12:21 pm on November 5, 2019: member

See #17300 (comment).

jonatack commented at 12:26 pm on November 5, 2019: member

@practicalswift Snap! :)

theStack commented at 1:17 pm on November 5, 2019: member

@practicalswift @jonatack Oops, I overlooked that. Mental note to myself: also search through merged/closed PRs about a change that is likely to having been discussed already, not only currently open ones. Thanks for pointing out!

Sjors commented at 1:42 pm on November 5, 2019: member

From the earlier comment:

Some of the unused spk_man variables in the warnings are used later in 07e0c23 from #17261 for locking cs_keystore

You could add a function void RequireLegacyScriptPubKeyMan() that only serves to check the presence. That in turn can call GetLegacyScriptPubKeyMan(). Later commits can then switch RequireLegacyScriptPubKeyMan() to GetLegacyScriptPubKeyMan() if they need the return value.

cc @achow101 @ryanofsky

wallet: remove unused variable spk_man in import* RPCs

Each of the following RPC functions define a variable reference
"LegacyScriptPubKeyMan& spk_man" which is not used, leading to compiler
warnings:
- importprivkey
- importaddress
- importpubkey
- importwallet
- importmulti

The call to GetLegacyScriptPubKeyMan() still serves the purpose to throw an
error if the wallet doesn't have a LegacyScriptPubKeyMan instance. This commit
introduces a new function RequireLegacyScriptPubKeyMan() which in turn calls
GetLegacyScriptPubKeyMan() but discards its return value.

606422b7ad

theStack force-pushed on Nov 5, 2019

theStack commented at 2:23 pm on November 5, 2019: member

@Sjors: Good idea, I updated the commit and the PR description accordingly.

If someone else wants to work on this who has more background knowledge about future use etc., probably on the course of a larger cleanup PR (@ryanofsky, @achow101 ?), feel free to close it though – I wouldn’t have opened it in the first place if I knew about #17338 and #17300 (comment).

ryanofsky added this to the "PRs" column in a project

Sjors commented at 3:18 pm on November 5, 2019: member

~ACK 606422b~ (works, but prefer ryanofsky’s solution) ./configure -Werror=unused-variable is happy again. @MarcoFalke didn’t we have a Travis instance with that config at some point?

ryanofsky commented at 4:16 pm on November 5, 2019: member

Sorry for causing these warnings. Feel free to pull in changes from b07b07cd8779355ba1dd16e7eb4af42e0ae1c587 in #17381 to avoid conflicts with that PR. I think the changes in that commit are a little better than current changes here (606422b7adfebfe9f8cda0480f6fda739331848d), because they extend the getter function to cover rpcwallet as well as rpcdump, and use Ensure naming consistent with EnsureWalletIsAvailable and EnsureWalletIsUnlocked. They implement what I previously suggested to jonatack in #17338 (comment)

ryanofsky removed this from the "PRs" column in a project

Sjors commented at 4:38 pm on November 5, 2019: member

Agree @ryanofsky’s version is more thorough, so let’s go with that.

fanquake commented at 4:46 pm on November 5, 2019: member

Closing in favour of #17381.

fanquake closed this on Nov 5, 2019

laanwj commented at 2:34 pm on November 6, 2019: member

Whoops. We should probably merge something to get rid of these warnings to avoid people wasting their time creating PR after PR fixing them.

theStack deleted the branch on Dec 1, 2020

DrahtBot locked this on Feb 15, 2022

wallet: remove unused variable spk_man in import* RPCs #17377