Merge #19004: refactor: Replace const char* to std::string

c57f03c refactor: Replace const char* to std::string (Calvin Kim)

Pull request description:

  Rationale: Addresses #19000
  Some functions should be returning std::string instead of const char*.
  This commit changes that.

  Main benefits/reasoning:

  1.  The functions never return nullptr, so returning a string makes code at call sites easier to review (reviewers don't have to read the source code to verify that a nullptr is never returned)
  2. All call sites convert to string anyway

ACKs for top commit:
  MarcoFalke:
    re-ACK c57f03c (no changes since previous review) 🚃
  Empact:
    Fair enough, Code Review ACK bitcoin/bitcoin@c57f03c
  practicalswift:
    ACK c57f03c -- patch looks correct
  hebasto:
    re-ACK c57f03c

Tree-SHA512: 9ce99bb38fe399b54844315048204cafce0f27fd8f24cae357fa7ac6f5d8094d57bbf5f5c1f5878a65f2d35e4a3f95d527eb17f49250b690c591c0df86ca84fd

Author: MarcoFalke

src/bitcoin-cli.cpp

@@ -21,6 +21,7 @@
 #include <functional>
 #include <memory>
 #include <stdio.h>
+#include <string>
 #include <tuple>
 
 #include <event2/buffer.h>
@@ -158,7 +159,7 @@ struct HTTPReply
     std::string body;
 };
 
-static const char *http_errorstring(int code)
+static std::string http_errorstring(int code)
 {
     switch(code) {
 #if LIBEVENT_VERSION_NUMBER >= 0x02010300

src/core_read.cpp

@@ -19,6 +19,7 @@
 #include <boost/algorithm/string/split.hpp>
 
 #include <algorithm>
+#include <string>
 
 CScript ParseScript(const std::string& s)
 {
@@ -34,10 +35,9 @@ CScript ParseScript(const std::string& s)
             if (op < OP_NOP && op != OP_RESERVED)
                 continue;
 
-            const char* name = GetOpName(static_cast<opcodetype>(op));
-            if (strcmp(name, "OP_UNKNOWN") == 0)
+            std::string strName = GetOpName(static_cast<opcodetype>(op));
+            if (strName == "OP_UNKNOWN")
                 continue;
-            std::string strName(name);
             mapOpNames[strName] = static_cast<opcodetype>(op);
             // Convenience: OP_ADD and just ADD are both recognized:
             boost::algorithm::replace_first(strName, "OP_", "");

src/script/script.cpp

@@ -7,7 +7,9 @@
 
 #include <util/strencodings.h>
 
-const char* GetOpName(opcodetype opcode)
+#include <string>
+
+std::string GetOpName(opcodetype opcode)
 {
     switch (opcode)
     {

src/script/script.h

@@ -193,7 +193,7 @@ enum opcodetype
 // Maximum value that an opcode can be
 static const unsigned int MAX_OPCODE = OP_NOP10;
 
-const char* GetOpName(opcodetype opcode);
+std::string GetOpName(opcodetype opcode);
 
 class scriptnum_error : public std::runtime_error
 {

src/script/script_error.cpp

@@ -5,7 +5,9 @@
 
 #include <script/script_error.h>
 
-const char* ScriptErrorString(const ScriptError serror)
+#include <string>
+
+std::string ScriptErrorString(const ScriptError serror)
 {
     switch (serror)
     {

src/script/script_error.h

@@ -6,6 +6,8 @@
 #ifndef BITCOIN_SCRIPT_SCRIPT_ERROR_H
 #define BITCOIN_SCRIPT_SCRIPT_ERROR_H
 
+#include <string>
+
 typedef enum ScriptError_t
 {
     SCRIPT_ERR_OK = 0,
@@ -73,6 +75,6 @@ typedef enum ScriptError_t
 
 #define SCRIPT_ERR_LAST SCRIPT_ERR_ERROR_COUNT
 
-const char* ScriptErrorString(const ScriptError error);
+std::string ScriptErrorString(const ScriptError error);
 
 #endif // BITCOIN_SCRIPT_SCRIPT_ERROR_H

src/script/standard.cpp

@@ -9,6 +9,8 @@
 #include <pubkey.h>
 #include <script/script.h>
 
+#include <string>
+
 typedef std::vector<unsigned char> valtype;
 
 bool fAcceptDatacarrier = DEFAULT_ACCEPT_DATACARRIER;
@@ -25,7 +27,7 @@ WitnessV0ScriptHash::WitnessV0ScriptHash(const CScript& in)
     CSHA256().Write(in.data(), in.size()).Finalize(begin());
 }
 
-const char* GetTxnOutputType(txnouttype t)
+std::string GetTxnOutputType(txnouttype t)
 {
     switch (t)
     {
@@ -39,7 +41,7 @@ const char* GetTxnOutputType(txnouttype t)
     case TX_WITNESS_V0_SCRIPTHASH: return "witness_v0_scripthash";
     case TX_WITNESS_UNKNOWN: return "witness_unknown";
     }
-    return nullptr;
+    assert(false);
 }
 
 static bool MatchPayToPubkey(const CScript& script, valtype& pubkey)

src/script/standard.h

@@ -11,6 +11,8 @@
 
 #include <boost/variant.hpp>
 
+#include <string>
+
 
 static const bool DEFAULT_ACCEPT_DATACARRIER = true;
 
@@ -145,7 +147,7 @@ typedef boost::variant<CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash,
 bool IsValidDestination(const CTxDestination& dest);
 
 /** Get the name of a txnouttype as a C string, or nullptr if unknown. */
-const char* GetTxnOutputType(txnouttype t);
+std::string GetTxnOutputType(txnouttype t);
 
 /**
  * Parse a scriptPubKey and identify script type for standard scripts. If

src/test/script_tests.cpp

@@ -102,7 +102,7 @@ static ScriptErrorDesc script_errors[]={
     {SCRIPT_ERR_SIG_FINDANDDELETE, "SIG_FINDANDDELETE"},
 };
 
-static const char *FormatScriptError(ScriptError_t err)
+static std::string FormatScriptError(ScriptError_t err)
 {
     for (unsigned int i=0; i<ARRAYLEN(script_errors); ++i)
         if (script_errors[i].err == err)
@@ -134,7 +134,7 @@ void DoTest(const CScript& scriptPubKey, const CScript& scriptSig, const CScript
     CMutableTransaction tx = BuildSpendingTransaction(scriptSig, scriptWitness, txCredit);
     CMutableTransaction tx2 = tx;
     BOOST_CHECK_MESSAGE(VerifyScript(scriptSig, scriptPubKey, &scriptWitness, flags, MutableTransactionSignatureChecker(&tx, 0, txCredit.vout[0].nValue), &err) == expect, message);
-    BOOST_CHECK_MESSAGE(err == scriptError, std::string(FormatScriptError(err)) + " where " + std::string(FormatScriptError((ScriptError_t)scriptError)) + " expected: " + message);
+    BOOST_CHECK_MESSAGE(err == scriptError, FormatScriptError(err) + " where " + FormatScriptError((ScriptError_t)scriptError) + " expected: " + message);
 
     // Verify that removing flags from a passing test or adding flags to a failing test does not change the result.
     for (int i = 0; i < 16; ++i) {