[bitcoin/bitcoin] Refactoring CRPCCommand with enum category (#13945)

251Labs commented on this pull request.

I am leaning towards NACK unfortunately.

> I am doing this, in preparation to add attributes, likes visibility and state to CRPCCommand structure, to avoid using category names, for overridden informations like visibility or deprecation.

Although I agree that this could be useful, the introduction of a new source and header file `rpccategory` to fulfil the future objective of being able to extend `CRPCCommand` feels a bit like over-engineering in the current state 5980416.

I would really prefer that the pull request focusses on the end goal first by trying to achieve that specifically.

One way could be to add the intended new «visibility» attribute to `CRPCCommand` and initialize the various `CRPCCommand` tables accordingly and take it from there.

Can we do this without having to add `rpccategory`?

Please also consider my inline feedback about the implementation of `std::string Label(const RPCCategory category)` which addresses https://github.com/bitcoin/bitcoin/pull/13945#discussion_r209468620 and would allow you to address https://github.com/bitcoin/bitcoin/pull/13945#discussion_r209469015 (both of which are *valid* comments IMO).

> + HANDLE_CASE_RETURN(RPCCategory, blockchain);
+ HANDLE_CASE_RETURN(RPCCategory, control);
+ HANDLE_CASE_RETURN(RPCCategory, generating);
+ HANDLE_CASE_RETURN(RPCCategory, hidden);
+ HANDLE_CASE_RETURN(RPCCategory, mining);
+ HANDLE_CASE_RETURN(RPCCategory, network);
+ HANDLE_CASE_RETURN(RPCCategory, rawtransactions);
+ HANDLE_CASE_RETURN(RPCCategory, util);
+ HANDLE_CASE_RETURN(RPCCategory, wallet);
+ HANDLE_CASE_RETURN(RPCCategory, test);
+ // if you are missing a case, you’ll have a warning here
+ }
+ assert(0); // never fall down here

I would prefer a more straightforward implementation like the one below if we include a category label function:

std::string Label(const RPCCategory category)
switch (category) {
case RPCCategory::blockchain: return «Blockchain»;
case RPCCategory::control: return «Control»;
case RPCCategory::generating: return «Generating»;
case RPCCategory::hidden: return «Hidden»;
case RPCCategory::mining: return «Mining»;
case RPCCategory::network: return «Network»;
case RPCCategory::rawtransactions: return «Rawtransactions»;
case RPCCategory::util: return «Util»;
case RPCCategory::wallet: return «Wallet»;
case RPCCategory::zmq: return «Zmq»;
case RPCCategory::test: return «Test»;
default: assert(false);

which would also enable you to simplify: