Merge #17279: refactor: Remove redundant c_str() calls in formatting

c72906dcc11a73fa06a0adf97557fa756b551bee refactor: Remove redundant c_str() calls in formatting (Wladimir J. van der Laan)

Pull request description:

  Our formatter, tinyformat, *never* needs `c_str()` for strings. Still, many places call it redundantly, resulting in longer code and a slight overhead.

  Remove redundant `c_str()` calls for:

  - `strprintf`
  - `LogPrintf`
  - `tfm::format`

  (also, combined with #17095, I think this improves logging in case of unexpected embedded NULL characters)

ACKs for top commit:
  ryanofsky:
    Code review ACK c72906dcc11a73fa06a0adf97557fa756b551bee. Easy to review with `git log -p -n1 --word-diff-regex=. -U0 c72906dcc11a73fa06a0adf97557fa756b551bee`

Tree-SHA512: 9e21e7bed8aaff59b8b8aa11571396ddc265fb29608c2545b1fcdbbb36d65b37eb361db6688dd36035eab0c110f8de255375cfda50df3d9d7708bc092f67fefc

src/bench/bench_bitcoin.cpp

@@ -36,7 +36,7 @@ int main(int argc, char** argv)
     SetupBenchArgs();
     std::string error;
     if (!gArgs.ParseParameters(argc, argv, error)) {
-        tfm::format(std::cerr, "Error parsing command line arguments: %s\n", error.c_str());
+        tfm::format(std::cerr, "Error parsing command line arguments: %s\n", error);
         return EXIT_FAILURE;
     }
 
@@ -60,7 +60,7 @@ int main(int argc, char** argv)
 
     double scaling_factor;
     if (!ParseDouble(scaling_str, &scaling_factor)) {
-        tfm::format(std::cerr, "Error parsing scaling factor as double: %s\n", scaling_str.c_str());
+        tfm::format(std::cerr, "Error parsing scaling factor as double: %s\n", scaling_str);
         return EXIT_FAILURE;
     }
 

src/bitcoin-cli.cpp

@@ -105,7 +105,7 @@ static int AppInitRPC(int argc, char* argv[])
     SetupCliArgs();
     std::string error;
     if (!gArgs.ParseParameters(argc, argv, error)) {
-        tfm::format(std::cerr, "Error parsing command line arguments: %s\n", error.c_str());
+        tfm::format(std::cerr, "Error parsing command line arguments: %s\n", error);
         return EXIT_FAILURE;
     }
     if (argc < 2 || HelpRequested(gArgs) || gArgs.IsArgSet("-version")) {
@@ -119,7 +119,7 @@ static int AppInitRPC(int argc, char* argv[])
             strUsage += "\n" + gArgs.GetHelpMessage();
         }
 
-        tfm::format(std::cout, "%s", strUsage.c_str());
+        tfm::format(std::cout, "%s", strUsage);
         if (argc < 2) {
             tfm::format(std::cerr, "Error: too few parameters\n");
             return EXIT_FAILURE;
@@ -127,11 +127,11 @@ static int AppInitRPC(int argc, char* argv[])
         return EXIT_SUCCESS;
     }
     if (!CheckDataDirOption()) {
-        tfm::format(std::cerr, "Error: Specified data directory \"%s\" does not exist.\n", gArgs.GetArg("-datadir", "").c_str());
+        tfm::format(std::cerr, "Error: Specified data directory \"%s\" does not exist.\n", gArgs.GetArg("-datadir", ""));
         return EXIT_FAILURE;
     }
     if (!gArgs.ReadConfigFiles(error, true)) {
-        tfm::format(std::cerr, "Error reading configuration file: %s\n", error.c_str());
+        tfm::format(std::cerr, "Error reading configuration file: %s\n", error);
         return EXIT_FAILURE;
     }
     // Check for -chain, -testnet or -regtest parameter (BaseParams() calls are only valid after this clause)
@@ -393,7 +393,7 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co
         if (failedToGetAuthCookie) {
             throw std::runtime_error(strprintf(
                 "Could not locate RPC credentials. No authentication cookie could be found, and RPC password is not set.  See -rpcpassword and -stdinrpcpass.  Configuration file: (%s)",
-                GetConfigFile(gArgs.GetArg("-conf", BITCOIN_CONF_FILENAME)).string().c_str()));
+                GetConfigFile(gArgs.GetArg("-conf", BITCOIN_CONF_FILENAME)).string()));
         } else {
             throw std::runtime_error("Authorization failed: Incorrect rpcuser or rpcpassword");
         }
@@ -541,7 +541,7 @@ static int CommandLineRPC(int argc, char *argv[])
     }
 
     if (strPrint != "") {
-        tfm::format(nRet == 0 ? std::cout : std::cerr, "%s\n", strPrint.c_str());
+        tfm::format(nRet == 0 ? std::cout : std::cerr, "%s\n", strPrint);
     }
     return nRet;
 }

src/bitcoin-tx.cpp

@@ -83,7 +83,7 @@ static int AppInitRawTx(int argc, char* argv[])
     SetupBitcoinTxArgs();
     std::string error;
     if (!gArgs.ParseParameters(argc, argv, error)) {
-        tfm::format(std::cerr, "Error parsing command line arguments: %s\n", error.c_str());
+        tfm::format(std::cerr, "Error parsing command line arguments: %s\n", error);
         return EXIT_FAILURE;
     }
 
@@ -105,7 +105,7 @@ static int AppInitRawTx(int argc, char* argv[])
             "\n";
         strUsage += gArgs.GetHelpMessage();
 
-        tfm::format(std::cout, "%s", strUsage.c_str());
+        tfm::format(std::cout, "%s", strUsage);
 
         if (argc < 2) {
             tfm::format(std::cerr, "Error: too few parameters\n");
@@ -724,21 +724,21 @@ static void OutputTxJSON(const CTransaction& tx)
     TxToUniv(tx, uint256(), entry);
 
     std::string jsonOutput = entry.write(4);
-    tfm::format(std::cout, "%s\n", jsonOutput.c_str());
+    tfm::format(std::cout, "%s\n", jsonOutput);
 }
 
 static void OutputTxHash(const CTransaction& tx)
 {
     std::string strHexHash = tx.GetHash().GetHex(); // the hex-encoded transaction hash (aka the transaction id)
 
-    tfm::format(std::cout, "%s\n", strHexHash.c_str());
+    tfm::format(std::cout, "%s\n", strHexHash);
 }
 
 static void OutputTxHex(const CTransaction& tx)
 {
     std::string strHex = EncodeHexTx(tx);
 
-    tfm::format(std::cout, "%s\n", strHex.c_str());
+    tfm::format(std::cout, "%s\n", strHex);
 }
 
 static void OutputTx(const CTransaction& tx)
@@ -829,7 +829,7 @@ static int CommandLineRawTx(int argc, char* argv[])
     }
 
     if (strPrint != "") {
-        tfm::format(nRet == 0 ? std::cout : std::cerr, "%s\n", strPrint.c_str());
+        tfm::format(nRet == 0 ? std::cout : std::cerr, "%s\n", strPrint);
     }
     return nRet;
 }

src/bitcoin-wallet.cpp

@@ -36,7 +36,7 @@ static bool WalletAppInit(int argc, char* argv[])
     SetupWalletToolArgs();
     std::string error_message;
     if (!gArgs.ParseParameters(argc, argv, error_message)) {
-        tfm::format(std::cerr, "Error parsing command line arguments: %s\n", error_message.c_str());
+        tfm::format(std::cerr, "Error parsing command line arguments: %s\n", error_message);
         return false;
     }
     if (argc < 2 || HelpRequested(gArgs)) {
@@ -48,7 +48,7 @@ static bool WalletAppInit(int argc, char* argv[])
                                      "  bitcoin-wallet [options] <command>\n\n" +
                                      gArgs.GetHelpMessage();
 
-        tfm::format(std::cout, "%s", usage.c_str());
+        tfm::format(std::cout, "%s", usage);
         return false;
     }
 
@@ -56,7 +56,7 @@ static bool WalletAppInit(int argc, char* argv[])
     LogInstance().m_print_to_console = gArgs.GetBoolArg("-printtoconsole", gArgs.GetBoolArg("-debug", false));
 
     if (!CheckDataDirOption()) {
-        tfm::format(std::cerr, "Error: Specified data directory \"%s\" does not exist.\n", gArgs.GetArg("-datadir", "").c_str());
+        tfm::format(std::cerr, "Error: Specified data directory \"%s\" does not exist.\n", gArgs.GetArg("-datadir", ""));
         return false;
     }
     // Check for -testnet or -regtest parameter (Params() calls are only valid after this clause)
@@ -87,7 +87,7 @@ int main(int argc, char* argv[])
     for(int i = 1; i < argc; ++i) {
         if (!IsSwitchChar(argv[i][0])) {
             if (!method.empty()) {
-                tfm::format(std::cerr, "Error: two methods provided (%s and %s). Only one method should be provided.\n", method.c_str(), argv[i]);
+                tfm::format(std::cerr, "Error: two methods provided (%s and %s). Only one method should be provided.\n", method, argv[i]);
                 return EXIT_FAILURE;
             }
             method = argv[i];

src/bitcoind.cpp

@@ -70,7 +70,7 @@ static bool AppInit(int argc, char* argv[])
             strUsage += "\n" + gArgs.GetHelpMessage();
         }
 
-        tfm::format(std::cout, "%s", strUsage.c_str());
+        tfm::format(std::cout, "%s", strUsage);
         return true;
     }
 

src/init.cpp

@@ -943,7 +943,7 @@ bool AppInitParameterInteraction()
     }
 
     if (!fs::is_directory(GetBlocksDir())) {
-        return InitError(strprintf(_("Specified blocks directory \"%s\" does not exist.").translated, gArgs.GetArg("-blocksdir", "").c_str()));
+        return InitError(strprintf(_("Specified blocks directory \"%s\" does not exist.").translated, gArgs.GetArg("-blocksdir", "")));
     }
 
     // parse and validate enabled filter types

src/net.cpp

@@ -1474,7 +1474,7 @@ static void ThreadMapPort()
                 if (externalIPAddress[0]) {
                     CNetAddr resolved;
                     if (LookupHost(externalIPAddress, resolved, false)) {
-                        LogPrintf("UPnP: ExternalIPAddress = %s\n", resolved.ToString().c_str());
+                        LogPrintf("UPnP: ExternalIPAddress = %s\n", resolved.ToString());
                         AddLocal(resolved, LOCAL_UPNP);
                     }
                 } else {
@@ -2710,7 +2710,7 @@ void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg)
 {
     size_t nMessageSize = msg.data.size();
     size_t nTotalSize = nMessageSize + CMessageHeader::HEADER_SIZE;
-    LogPrint(BCLog::NET, "sending %s (%d bytes) peer=%d\n",  SanitizeString(msg.command.c_str()), nMessageSize, pnode->GetId());
+    LogPrint(BCLog::NET, "sending %s (%d bytes) peer=%d\n",  SanitizeString(msg.command), nMessageSize, pnode->GetId());
 
     std::vector<unsigned char> serializedHeader;
     serializedHeader.reserve(CMessageHeader::HEADER_SIZE);

src/noui.cpp

@@ -45,7 +45,7 @@ bool noui_ThreadSafeMessageBox(const std::string& message, const std::string& ca
     if (!fSecure) {
         LogPrintf("%s%s\n", strCaption, message);
     }
-    tfm::format(std::cerr, "%s%s\n", strCaption.c_str(), message.c_str());
+    tfm::format(std::cerr, "%s%s\n", strCaption, message);
     return false;
 }
 

src/policy/fees.cpp

@@ -517,7 +517,7 @@ void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, boo
     uint256 hash = entry.GetTx().GetHash();
     if (mapMemPoolTxs.count(hash)) {
         LogPrint(BCLog::ESTIMATEFEE, "Blockpolicy error mempool tx %s already being tracked\n",
-                 hash.ToString().c_str());
+                 hash.ToString());
         return;
     }
 

src/rpc/rawtransaction.cpp

@@ -1602,7 +1602,7 @@ UniValue joinpsbts(const JSONRPCRequest& request)
     for (auto& psbt : psbtxs) {
         for (unsigned int i = 0; i < psbt.tx->vin.size(); ++i) {
             if (!merged_psbt.AddInput(psbt.tx->vin[i], psbt.inputs[i])) {
-                throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Input %s:%d exists in multiple PSBTs", psbt.tx->vin[i].prevout.hash.ToString().c_str(), psbt.tx->vin[i].prevout.n));
+                throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Input %s:%d exists in multiple PSBTs", psbt.tx->vin[i].prevout.hash.ToString(), psbt.tx->vin[i].prevout.n));
             }
         }
         for (unsigned int i = 0; i < psbt.tx->vout.size(); ++i) {

src/script/descriptor.cpp

@@ -645,7 +645,7 @@ NODISCARD bool ParseKeyPath(const std::vector<Span<const char>>& split, KeyPath&
         }
         uint32_t p;
         if (!ParseUInt32(std::string(elem.begin(), elem.end()), &p)) {
-            error = strprintf("Key path value '%s' is not a valid uint32", std::string(elem.begin(), elem.end()).c_str());
+            error = strprintf("Key path value '%s' is not a valid uint32", std::string(elem.begin(), elem.end()));
             return false;
         } else if (p > 0x7FFFFFFFUL) {
             error = strprintf("Key path value %u is out of range", p);
@@ -783,7 +783,7 @@ std::unique_ptr<DescriptorImpl> ParseScript(Span<const char>& sp, ParseScriptCon
         uint32_t thres;
         std::vector<std::unique_ptr<PubkeyProvider>> providers;
         if (!ParseUInt32(std::string(threshold.begin(), threshold.end()), &thres)) {
-            error = strprintf("Multi threshold '%s' is not valid", std::string(threshold.begin(), threshold.end()).c_str());
+            error = strprintf("Multi threshold '%s' is not valid", std::string(threshold.begin(), threshold.end()));
             return nullptr;
         }
         size_t script_size = 0;

src/sync.cpp

@@ -173,7 +173,7 @@ void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine,
     for (const std::pair<void*, CLockLocation>& i : g_lockstack)
         if (i.first == cs)
             return;
-    tfm::format(std::cerr, "Assertion failed: lock %s not held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld().c_str());
+    tfm::format(std::cerr, "Assertion failed: lock %s not held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld());
     abort();
 }
 
@@ -181,7 +181,7 @@ void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLi
 {
     for (const std::pair<void*, CLockLocation>& i : g_lockstack) {
         if (i.first == cs) {
-            tfm::format(std::cerr, "Assertion failed: lock %s held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld().c_str());
+            tfm::format(std::cerr, "Assertion failed: lock %s held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld());
             abort();
         }
     }

src/util/system.cpp

@@ -305,7 +305,7 @@ NODISCARD static bool InterpretOption(std::string key, std::string val, unsigned
             LogPrintf("Warning: parsed potentially confusing double-negative %s=%s\n", key, val);
             val = "1";
         } else {
-            error = strprintf("Negating of %s is meaningless and therefore forbidden", key.c_str());
+            error = strprintf("Negating of %s is meaningless and therefore forbidden", key);
             return false;
         }
     }
@@ -414,7 +414,7 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin
                 return false;
             }
         } else {
-            error = strprintf("Invalid parameter %s", key.c_str());
+            error = strprintf("Invalid parameter %s", key);
             return false;
         }
     }
@@ -688,7 +688,7 @@ void PrintExceptionContinue(const std::exception* pex, const char* pszThread)
 {
     std::string message = FormatException(pex, pszThread);
     LogPrintf("\n\n************************\n%s\n", message);
-    tfm::format(std::cerr, "\n\n************************\n%s\n", message.c_str());
+    tfm::format(std::cerr, "\n\n************************\n%s\n", message);
 }
 
 fs::path GetDefaultDataDir()
@@ -870,7 +870,7 @@ bool ArgsManager::ReadConfigStream(std::istream& stream, const std::string& file
             if (ignore_invalid_keys) {
                 LogPrintf("Ignoring unknown configuration value %s\n", option.first);
             } else {
-                error = strprintf("Invalid configuration value %s", option.first.c_str());
+                error = strprintf("Invalid configuration value %s", option.first);
                 return false;
             }
         }
@@ -925,7 +925,7 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
                     if (!ReadConfigStream(include_config, to_include, error, ignore_invalid_keys)) {
                         return false;
                     }
-                    LogPrintf("Included configuration file %s\n", to_include.c_str());
+                    LogPrintf("Included configuration file %s\n", to_include);
                 } else {
                     error = "Failed to include configuration file " + to_include;
                     return false;
@@ -945,7 +945,7 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
                 }
             }
             for (const std::string& to_include : includeconf) {
-                tfm::format(std::cerr, "warning: -includeconf cannot be used from included files; ignoring -includeconf=%s\n", to_include.c_str());
+                tfm::format(std::cerr, "warning: -includeconf cannot be used from included files; ignoring -includeconf=%s\n", to_include);
             }
         }
     }
@@ -953,7 +953,7 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
     // If datadir is changed in .conf file:
     ClearDatadirCache();
     if (!CheckDataDirOption()) {
-        error = strprintf("specified data directory \"%s\" does not exist.", gArgs.GetArg("-datadir", "").c_str());
+        error = strprintf("specified data directory \"%s\" does not exist.", gArgs.GetArg("-datadir", ""));
         return false;
     }
     return true;

src/wallet/wallettool.cpp

@@ -30,7 +30,7 @@ static std::shared_ptr<CWallet> CreateWallet(const std::string& name, const fs::
     bool first_run = true;
     DBErrors load_wallet_ret = wallet_instance->LoadWallet(first_run);
     if (load_wallet_ret != DBErrors::LOAD_OK) {
-        tfm::format(std::cerr, "Error creating %s", name.c_str());
+        tfm::format(std::cerr, "Error creating %s", name);
         return nullptr;
     }
 
@@ -59,28 +59,28 @@ static std::shared_ptr<CWallet> LoadWallet(const std::string& name, const fs::pa
         bool first_run;
         load_wallet_ret = wallet_instance->LoadWallet(first_run);
     } catch (const std::runtime_error&) {
-        tfm::format(std::cerr, "Error loading %s. Is wallet being used by another process?\n", name.c_str());
+        tfm::format(std::cerr, "Error loading %s. Is wallet being used by another process?\n", name);
         return nullptr;
     }
 
     if (load_wallet_ret != DBErrors::LOAD_OK) {
         wallet_instance = nullptr;
         if (load_wallet_ret == DBErrors::CORRUPT) {
-            tfm::format(std::cerr, "Error loading %s: Wallet corrupted", name.c_str());
+            tfm::format(std::cerr, "Error loading %s: Wallet corrupted", name);
             return nullptr;
         } else if (load_wallet_ret == DBErrors::NONCRITICAL_ERROR) {
             tfm::format(std::cerr, "Error reading %s! All keys read correctly, but transaction data"
                             " or address book entries might be missing or incorrect.",
-                name.c_str());
+                name);
         } else if (load_wallet_ret == DBErrors::TOO_NEW) {
             tfm::format(std::cerr, "Error loading %s: Wallet requires newer version of %s",
-                name.c_str(), PACKAGE_NAME);
+                name, PACKAGE_NAME);
             return nullptr;
         } else if (load_wallet_ret == DBErrors::NEED_REWRITE) {
             tfm::format(std::cerr, "Wallet needed to be rewritten: restart %s to complete", PACKAGE_NAME);
             return nullptr;
         } else {
-            tfm::format(std::cerr, "Error loading %s", name.c_str());
+            tfm::format(std::cerr, "Error loading %s", name);
             return nullptr;
         }
     }
@@ -112,12 +112,12 @@ bool ExecuteWalletToolFunc(const std::string& command, const std::string& name)
         }
     } else if (command == "info") {
         if (!fs::exists(path)) {
-            tfm::format(std::cerr, "Error: no wallet file at %s\n", name.c_str());
+            tfm::format(std::cerr, "Error: no wallet file at %s\n", name);
             return false;
         }
         std::string error;
         if (!WalletBatch::VerifyEnvironment(path, error)) {
-            tfm::format(std::cerr, "Error loading %s. Is wallet being used by other process?\n", name.c_str());
+            tfm::format(std::cerr, "Error loading %s. Is wallet being used by other process?\n", name);
             return false;
         }
         std::shared_ptr<CWallet> wallet_instance = LoadWallet(name, path);
@@ -125,7 +125,7 @@ bool ExecuteWalletToolFunc(const std::string& command, const std::string& name)
         WalletShowInfo(wallet_instance.get());
         wallet_instance->Flush(true);
     } else {
-        tfm::format(std::cerr, "Invalid command: %s\n", command.c_str());
+        tfm::format(std::cerr, "Invalid command: %s\n", command);
         return false;
     }