Merge bitcoin/bitcoin#29040: refactor: Remove pre-C++20 code, fs::path cleanup

achow101 · achow101 · commit 1b2dedbf5c45 · 2023-12-14T16:46:54.000-05:00
6666713 refactor: Rename fs::path::u8string() to fs::path::utf8string() (MarcoFalke) 856c887 ArgsManager: return path by value from GetBlocksDirPath() (Vasil Dimov) fa3d930 refactor: Remove pre-C++20 fs code (MarcoFalke) fa00098 Add tests for C++20 std::u8string (MarcoFalke) fa2bac0 refactor: Avoid copy/move in fs.h (MarcoFalke) faea302 refactor: Use C++20 std::chrono::days (MarcoFalke) Pull request description: This: * Removes dead code. * Avoids unused copies in some places. * Adds copies in other places for safety. ACKs for top commit: achow101: ACK 6666713 ryanofsky: Code review ACK 6666713. Just documentation change since last review. stickies-v: re-ACK 6666713 Tree-SHA512: 6176e44f30b310d51632ec2d3827c3819905d0ddc6a4b57acfcb6cfa1f9735176da75ee8ed4a4abd1296cb0b83bee9374cc6f91ffac87c19b63c435eeadf3f46
diff --git a/doc/developer-notes.md b/doc/developer-notes.md
@@ -1406,7 +1406,7 @@ A few guidelines for introducing and reviewing new RPC interfaces:
 
   - *Rationale*: User-facing consistency.
 
-- Use `fs::path::u8string()` and `fs::u8path()` functions when converting path
+- Use `fs::path::u8string()`/`fs::path::utf8string()` and `fs::u8path()` functions when converting path
   to JSON strings, not `fs::PathToString` and `fs::PathFromString`
 
   - *Rationale*: JSON strings are Unicode strings, not byte strings, and
diff --git a/src/bench/wallet_create.cpp b/src/bench/wallet_create.cpp
@@ -34,7 +34,7 @@ static void WalletCreate(benchmark::Bench& bench, bool encrypted)
 
     fs::path wallet_path = test_setup->m_path_root / strprintf("test_wallet_%d", random.rand32()).c_str();
     bench.run([&] {
-        auto wallet = CreateWallet(context, wallet_path.u8string(), /*load_on_start=*/std::nullopt, options, status, error_string, warnings);
+        auto wallet = CreateWallet(context, wallet_path.utf8string(), /*load_on_start=*/std::nullopt, options, status, error_string, warnings);
         assert(status == DatabaseStatus::SUCCESS);
         assert(wallet != nullptr);
 
diff --git a/src/common/args.cpp b/src/common/args.cpp
@@ -277,7 +277,7 @@ fs::path ArgsManager::GetPathArg(std::string arg, const fs::path& default_value)
     return result.has_filename() ? result : result.parent_path();
 }
 
-const fs::path& ArgsManager::GetBlocksDirPath() const
+fs::path ArgsManager::GetBlocksDirPath() const
 {
     LOCK(cs_args);
     fs::path& path = m_cached_blocks_path;
@@ -302,7 +302,7 @@ const fs::path& ArgsManager::GetBlocksDirPath() const
     return path;
 }
 
-const fs::path& ArgsManager::GetDataDir(bool net_specific) const
+fs::path ArgsManager::GetDataDir(bool net_specific) const
 {
     LOCK(cs_args);
     fs::path& path = net_specific ? m_cached_network_datadir_path : m_cached_datadir_path;
diff --git a/src/common/args.h b/src/common/args.h
@@ -215,21 +215,21 @@ class ArgsManager
      *
      * @return Blocks path which is network specific
      */
-    const fs::path& GetBlocksDirPath() const;
+    fs::path GetBlocksDirPath() const;
 
     /**
      * Get data directory path
      *
      * @return Absolute path on success, otherwise an empty path when a non-directory path would be returned
      */
-    const fs::path& GetDataDirBase() const { return GetDataDir(false); }
+    fs::path GetDataDirBase() const { return GetDataDir(false); }
 
     /**
      * Get data directory path with appended network identifier
      *
      * @return Absolute path on success, otherwise an empty path when a non-directory path would be returned
      */
-    const fs::path& GetDataDirNet() const { return GetDataDir(true); }
+    fs::path GetDataDirNet() const { return GetDataDir(true); }
 
     /**
      * Clear cached directory paths
@@ -420,7 +420,7 @@ class ArgsManager
      * @param net_specific Append network identifier to the returned path
      * @return Absolute path on success, otherwise an empty path when a non-directory path would be returned
      */
-    const fs::path& GetDataDir(bool net_specific) const;
+    fs::path GetDataDir(bool net_specific) const;
 
     /**
      * Return -regtest/-signet/-testnet/-chain= setting as a ChainType enum if a
diff --git a/src/qt/guiutil.cpp b/src/qt/guiutil.cpp
@@ -669,7 +669,7 @@ fs::path QStringToPath(const QString &path)
 
 QString PathToQString(const fs::path &path)
 {
-    return QString::fromStdString(path.u8string());
+    return QString::fromStdString(path.utf8string());
 }
 
 QString NetworkToQString(Network net)
@@ -723,8 +723,7 @@ QString ConnectionTypeToQString(ConnectionType conn_type, bool prepend_direction
 
 QString formatDurationStr(std::chrono::seconds dur)
 {
-    using days = std::chrono::duration<int, std::ratio<86400>>; // can remove this line after C++20
-    const auto d{std::chrono::duration_cast<days>(dur)};
+    const auto d{std::chrono::duration_cast<std::chrono::days>(dur)};
     const auto h{std::chrono::duration_cast<std::chrono::hours>(dur - d)};
     const auto m{std::chrono::duration_cast<std::chrono::minutes>(dur - d - h)};
     const auto s{std::chrono::duration_cast<std::chrono::seconds>(dur - d - h - m)};
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
@@ -2601,7 +2601,7 @@ static RPCHelpMan dumptxoutset()
     if (fs::exists(path)) {
         throw JSONRPCError(
             RPC_INVALID_PARAMETER,
-            path.u8string() + " already exists. If you are sure this is what you want, "
+            path.utf8string() + " already exists. If you are sure this is what you want, "
             "move it out of the way first");
     }
 
@@ -2610,15 +2610,15 @@ static RPCHelpMan dumptxoutset()
     if (afile.IsNull()) {
         throw JSONRPCError(
             RPC_INVALID_PARAMETER,
-            "Couldn't open file " + temppath.u8string() + " for writing.");
+            "Couldn't open file " + temppath.utf8string() + " for writing.");
     }
 
     NodeContext& node = EnsureAnyNodeContext(request.context);
     UniValue result = CreateUTXOSnapshot(
         node, node.chainman->ActiveChainstate(), afile, path, temppath);
     fs::rename(temppath, path);
 
-    result.pushKV("path", path.u8string());
+    result.pushKV("path", path.utf8string());
     return result;
 },
     };
@@ -2690,7 +2690,7 @@ UniValue CreateUTXOSnapshot(
     result.pushKV("coins_written", maybe_stats->coins_count);
     result.pushKV("base_hash", tip->GetBlockHash().ToString());
     result.pushKV("base_height", tip->nHeight);
-    result.pushKV("path", path.u8string());
+    result.pushKV("path", path.utf8string());
     result.pushKV("txoutset_hash", maybe_stats->hashSerialized.ToString());
     result.pushKV("nchaintx", tip->nChainTx);
     return result;
@@ -2743,7 +2743,7 @@ static RPCHelpMan loadtxoutset()
     if (afile.IsNull()) {
         throw JSONRPCError(
             RPC_INVALID_PARAMETER,
-            "Couldn't open file " + path.u8string() + " for reading.");
+            "Couldn't open file " + path.utf8string() + " for reading.");
     }
 
     SnapshotMetadata metadata;
diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
@@ -806,7 +806,7 @@ static RPCHelpMan savemempool()
     }
 
     UniValue ret(UniValue::VOBJ);
-    ret.pushKV("filename", dump_path.u8string());
+    ret.pushKV("filename", dump_path.utf8string());
 
     return ret;
 },
diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
@@ -245,7 +245,7 @@ static RPCHelpMan getrpcinfo()
     UniValue result(UniValue::VOBJ);
     result.pushKV("active_commands", active_commands);
 
-    const std::string path = LogInstance().m_file_path.u8string();
+    const std::string path = LogInstance().m_file_path.utf8string();
     UniValue log_path(UniValue::VSTR, path);
     result.pushKV("logpath", log_path);
 
diff --git a/src/test/fs_tests.cpp b/src/test/fs_tests.cpp
@@ -17,9 +17,12 @@ BOOST_FIXTURE_TEST_SUITE(fs_tests, BasicTestingSetup)
 BOOST_AUTO_TEST_CASE(fsbridge_pathtostring)
 {
     std::string u8_str = "fs_tests_₿_🏃";
+    std::u8string str8{u8"fs_tests_₿_🏃"};
     BOOST_CHECK_EQUAL(fs::PathToString(fs::PathFromString(u8_str)), u8_str);
-    BOOST_CHECK_EQUAL(fs::u8path(u8_str).u8string(), u8_str);
-    BOOST_CHECK_EQUAL(fs::PathFromString(u8_str).u8string(), u8_str);
+    BOOST_CHECK_EQUAL(fs::u8path(u8_str).utf8string(), u8_str);
+    BOOST_CHECK_EQUAL(fs::path(str8).utf8string(), u8_str);
+    BOOST_CHECK(fs::path(str8).u8string() == str8);
+    BOOST_CHECK_EQUAL(fs::PathFromString(u8_str).utf8string(), u8_str);
     BOOST_CHECK_EQUAL(fs::PathToString(fs::u8path(u8_str)), u8_str);
 #ifndef WIN32
     // On non-windows systems, verify that arbitrary byte strings containing
@@ -46,7 +49,7 @@ BOOST_AUTO_TEST_CASE(fsbridge_fstream)
     fs::path tmpfolder = m_args.GetDataDirBase();
     // tmpfile1 should be the same as tmpfile2
     fs::path tmpfile1 = tmpfolder / fs::u8path("fs_tests_₿_🏃");
-    fs::path tmpfile2 = tmpfolder / fs::u8path("fs_tests_₿_🏃");
+    fs::path tmpfile2 = tmpfolder / fs::path(u8"fs_tests_₿_🏃");
     {
         std::ofstream file{tmpfile1};
         file << "bitcoin";
diff --git a/src/util/fs.cpp b/src/util/fs.cpp
@@ -1,4 +1,4 @@
-// Copyright (c) 2017-2022 The Bitcoin Core developers
+// Copyright (c) 2017-present The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
@@ -18,6 +18,7 @@
 #endif
 
 #include <cassert>
+#include <cerrno>
 #include <string>
 
 namespace fsbridge {
@@ -130,4 +131,4 @@ std::string get_filesystem_error_message(const fs::filesystem_error& e)
 #endif
 }
 
-} // fsbridge
+} // namespace fsbridge
diff --git a/src/util/fs.h b/src/util/fs.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2017-2022 The Bitcoin Core developers
+// Copyright (c) 2017-present The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
@@ -14,6 +14,8 @@
 #include <ios>
 #include <ostream>
 #include <string>
+#include <system_error>
+#include <type_traits>
 #include <utility>
 
 /** Filesystem operations and types */
@@ -35,7 +37,7 @@ class path : public std::filesystem::path
     // Allow path objects arguments for compatibility.
     path(std::filesystem::path path) : std::filesystem::path::path(std::move(path)) {}
     path& operator=(std::filesystem::path path) { std::filesystem::path::operator=(std::move(path)); return *this; }
-    path& operator/=(std::filesystem::path path) { std::filesystem::path::operator/=(path); return *this; }
+    path& operator/=(const std::filesystem::path& path) { std::filesystem::path::operator/=(path); return *this; }
 
     // Allow literal string arguments, which are safe as long as the literals are ASCII.
     path(const char* c) : std::filesystem::path(c) {}
@@ -52,12 +54,15 @@ class path : public std::filesystem::path
     // Disallow std::string conversion method to avoid locale-dependent encoding on windows.
     std::string string() const = delete;
 
-    std::string u8string() const
+    /**
+     * Return a UTF-8 representation of the path as a std::string, for
+     * compatibility with code using std::string. For code using the newer
+     * std::u8string type, it is more efficient to call the inherited
+     * std::filesystem::path::u8string method instead.
+     */
+    std::string utf8string() const
     {
-        const auto& utf8_str{std::filesystem::path::u8string()};
-        // utf8_str might either be std::string (C++17) or std::u8string
-        // (C++20). Convert both to std::string. This method can be removed
-        // after switching to C++20.
+        const std::u8string& utf8_str{std::filesystem::path::u8string()};
         return std::string{utf8_str.begin(), utf8_str.end()};
     }
 
@@ -69,11 +74,7 @@ class path : public std::filesystem::path
 
 static inline path u8path(const std::string& utf8_str)
 {
-#if __cplusplus < 202002L
-    return std::filesystem::u8path(utf8_str);
-#else
     return std::filesystem::path(std::u8string{utf8_str.begin(), utf8_str.end()});
-#endif
 }
 
 // Disallow implicit std::string conversion for absolute to avoid
@@ -97,9 +98,9 @@ static inline auto quoted(const std::string& s)
 }
 
 // Allow safe path append operations.
-static inline path operator/(path p1, path p2)
+static inline path operator/(path p1, const path& p2)
 {
-    p1 /= std::move(p2);
+    p1 /= p2;
     return p1;
 }
 static inline path operator/(path p1, const char* p2)
@@ -140,7 +141,7 @@ static inline bool copy_file(const path& from, const path& to, copy_options opti
  * Because \ref PathToString and \ref PathFromString functions don't specify an
  * encoding, they are meant to be used internally, not externally. They are not
  * appropriate to use in applications requiring UTF-8, where
- * fs::path::u8string() and fs::u8path() methods should be used instead. Other
+ * fs::path::u8string() / fs::path::utf8string() and fs::u8path() methods should be used instead. Other
  * applications could require still different encodings. For example, JSON, XML,
  * or URI applications might prefer to use higher-level escapes (\uXXXX or
  * &XXXX; or %XX) instead of multibyte encoding. Rust, Python, Java applications
@@ -154,13 +155,13 @@ static inline std::string PathToString(const path& path)
     // use here, because these methods encode the path using C++'s narrow
     // multibyte encoding, which on Windows corresponds to the current "code
     // page", which is unpredictable and typically not able to represent all
-    // valid paths. So fs::path::u8string() and
+    // valid paths. So fs::path::utf8string() and
     // fs::u8path() functions are used instead on Windows. On
-    // POSIX, u8string/u8path functions are not safe to use because paths are
+    // POSIX, u8string/utf8string/u8path functions are not safe to use because paths are
     // not always valid UTF-8, so plain string methods which do not transform
     // the path there are used.
 #ifdef WIN32
-    return path.u8string();
+    return path.utf8string();
 #else
     static_assert(std::is_same<path::string_type, std::string>::value, "PathToString not implemented on this platform");
     return path.std::filesystem::path::string();
diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
@@ -734,7 +734,7 @@ RPCHelpMan dumpwallet()
      * It may also avoid other security issues.
      */
     if (fs::exists(filepath)) {
-        throw JSONRPCError(RPC_INVALID_PARAMETER, filepath.u8string() + " already exists. If you are sure this is what you want, move it out of the way first");
+        throw JSONRPCError(RPC_INVALID_PARAMETER, filepath.utf8string() + " already exists. If you are sure this is what you want, move it out of the way first");
     }
 
     std::ofstream file;
@@ -828,7 +828,7 @@ RPCHelpMan dumpwallet()
     file.close();
 
     UniValue reply(UniValue::VOBJ);
-    reply.pushKV("filename", filepath.u8string());
+    reply.pushKV("filename", filepath.utf8string());
 
     return reply;
 },
diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp
@@ -169,7 +169,7 @@ static RPCHelpMan listwalletdir()
     UniValue wallets(UniValue::VARR);
     for (const auto& path : ListDatabases(GetWalletDir())) {
         UniValue wallet(UniValue::VOBJ);
-        wallet.pushKV("name", path.u8string());
+        wallet.pushKV("name", path.utf8string());
         wallets.push_back(wallet);
     }
 
@@ -806,7 +806,7 @@ static RPCHelpMan migratewallet()
             if (res->solvables_wallet) {
                 r.pushKV("solvables_name", res->solvables_wallet->GetName());
             }
-            r.pushKV("backup_path", res->backup_path.u8string());
+            r.pushKV("backup_path", res->backup_path.utf8string());
 
             return r;
         },

Original file line number	Diff line number	Diff line change
`@@ -277,7 +277,7 @@ fs::path ArgsManager::GetPathArg(std::string arg, const fs::path& default_value)`
`277`	`277`	`return result.has_filename() ? result : result.parent_path();`
`278`	`278`	`}`
`279`	`279`
`280`		`-const fs::path& ArgsManager::GetBlocksDirPath() const`
	`280`	`+fs::path ArgsManager::GetBlocksDirPath() const`
`281`	`281`	`{`
`282`	`282`	`LOCK(cs_args);`
`283`	`283`	`fs::path& path = m_cached_blocks_path;`
`@@ -302,7 +302,7 @@ const fs::path& ArgsManager::GetBlocksDirPath() const`
`302`	`302`	`return path;`
`303`	`303`	`}`
`304`	`304`
`305`		`-const fs::path& ArgsManager::GetDataDir(bool net_specific) const`
	`305`	`+fs::path ArgsManager::GetDataDir(bool net_specific) const`
`306`	`306`	`{`
`307`	`307`	`LOCK(cs_args);`
`308`	`308`	`fs::path& path = net_specific ? m_cached_network_datadir_path : m_cached_datadir_path;`
Original file line number	Diff line number	Diff line change
`@@ -669,7 +669,7 @@ fs::path QStringToPath(const QString &path)`
`669`	`669`
`670`	`670`	`QString PathToQString(const fs::path &path)`
`671`	`671`	`{`
`672`		`- return QString::fromStdString(path.u8string());`
	`672`	`+ return QString::fromStdString(path.utf8string());`
`673`	`673`	`}`
`674`	`674`
`675`	`675`	`QString NetworkToQString(Network net)`
`@@ -723,8 +723,7 @@ QString ConnectionTypeToQString(ConnectionType conn_type, bool prepend_direction`
`723`	`723`
`724`	`724`	`QString formatDurationStr(std::chrono::seconds dur)`
`725`	`725`	`{`
`726`		`- using days = std::chrono::duration<int, std::ratio<86400>>; // can remove this line after C++20`
`727`		`- const auto d{std::chrono::duration_cast<days>(dur)};`
	`726`	`+ const auto d{std::chrono::duration_cast<std::chrono::days>(dur)};`
`728`	`727`	`const auto h{std::chrono::duration_cast<std::chrono::hours>(dur - d)};`
`729`	`728`	`const auto m{std::chrono::duration_cast<std::chrono::minutes>(dur - d - h)};`
`730`	`729`	`const auto s{std::chrono::duration_cast<std::chrono::seconds>(dur - d - h - m)};`
Original file line number	Diff line number	Diff line change
`@@ -806,7 +806,7 @@ static RPCHelpMan savemempool()`
`806`	`806`	`}`
`807`	`807`
`808`	`808`	`UniValue ret(UniValue::VOBJ);`
`809`		`- ret.pushKV("filename", dump_path.u8string());`
	`809`	`+ ret.pushKV("filename", dump_path.utf8string());`
`810`	`810`
`811`	`811`	`return ret;`
`812`	`812`	`},`