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

251Labs commented on this pull request.

Between NACK / weak concept ACK b852456.

I still have the same reservations that made me NACK this before, but b852456 looks better.

I have added some additional review comments that are hopefully helpful.

> +// Copyright (c) 2018 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include
+
+#include
+
+namespace rpccategory
+{
+
+std::string Label(const RPCCategory category)
+{
+ switch (category) {
+ case RPCCategory::BLOCKCHAIN: return «blockchain»;
+ case RPCCategory::CONTROL: return «control»;

If we introduce a freestanding utility function that returns category labels, I think we should at least make it return the correct labels:

«`c++
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»;
«`

> +
+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»;
+ // if you are missing a case, you’ll have a warning here

nit: remove comment?

> @@ -0,0 +1,32 @@
+// Copyright (c) 2018 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_RPC_RPCCATEGORY_H
+#define BITCOIN_RPC_RPCCATEGORY_H
+
+#include
+
+enum class RPCCategory

To what extend are `RPCCategory` and `OptionsCategory` in `util.h` redundant?

> @@ -187,11 +187,11 @@ std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest&
if (strHelp.find(‘\n’) != std::string::npos)
strHelp = strHelp.substr(0, strHelp.find(‘\n’));

— if (category != pcmd->category)
+ if (category != rpccategory::Label(pcmd->category))

If we introduce RPC category enumerations, I think we should also use them and not map their values back to labels.

By declaring `std::string category` as `RPCCategory category` and making `rpccategory::Label` return the correct labels you will be able to optimize this `if` statement:

«`c++
if (category != rpccategory::Label(pcmd->category))
{
if (!category.empty())
strRet += «\n»;
category = rpccategory::Label(pcmd->category);
std::string firstLetter = category.substr(0,1);
boost::to_upper(firstLetter);
strRet += «== » + firstLetter + category.substr(1) + » ==\n»;
}
«`

to:

«`c++
if (category != pcmd->category) {
category = pcmd->category;
strRet += «\n== » + rpccategory::Label(category) + » ==\n»;
}
«`

and remove:

«`c++
#include // for to_upper()
«`

> @@ -131,10 +133,10 @@ typedef UniValue(*rpcfn_type)(const JSONRPCRequest& jsonRequest);
class CRPCCommand
{
public:

nit: while we are here, maybe make `CRPCCommand` a struct and remove the `public` access modifier?