diff options
| author | Ryan Ofsky <ryan@ofsky.org> | 2024-08-06 14:30:49 -0400 |
|---|---|---|
| committer | Ryan Ofsky <ryan@ofsky.org> | 2024-08-06 15:41:38 -0400 |
| commit | bb25e0691f656b7a971e0fb4d52ddda9eb4a2604 (patch) | |
| tree | 86a3b6ca38d5ed1888fe30e1afcce2b903e38213 | |
| parent | d7333ece15b8a9c941af072cebc210b099900d0e (diff) | |
| parent | fa18fc705084f3620be566d8c6639b29117ccf68 (diff) | |
Merge bitcoin/bitcoin#30485: log: Remove NOLINT(bitcoin-unterminated-logprintf)
fa18fc705084f3620be566d8c6639b29117ccf68 log: Remove NOLINT(bitcoin-unterminated-logprintf) (MarcoFalke)
Pull request description:
`NOLINT(bitcoin-unterminated-logprintf)` is used to document a missing trailing `\n` char in the format string. This has many issues:
* It is just documentation, assuming that a trailing `\n` ends up in the formatted string. It is not enforced at compile-time, so it is brittle.
* If the newline was truly missing and `NOLINT(bitcoin-unterminated-logprintf)` were used to document a "continued" line, the log stream would be racy/corrupt, because any other thread may inject a log message in the meantime.
* If the newline was accidentally missing, nothing is there to correct the mistake.
* The intention of all code is to always end a log line with a new line. However, historic code exists to deal with the case where the new line was missing (`m_started_new_line`). This is problematic, because the presumed dead code has to be maintained (https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1682963306).
Fix almost all issues by removing the `NOLINT(bitcoin-unterminated-logprintf)`, ensuring that a new line is always present.
A follow-up will remove the dead logging code.
ACKs for top commit:
TheCharlatan:
ACK fa18fc705084f3620be566d8c6639b29117ccf68
ryanofsky:
Code review ACK fa18fc705084f3620be566d8c6639b29117ccf68
Tree-SHA512: bf8a83723cca84e21187658edc19612da79c34f7ef2e1f6e9353e7ba70e4ecc0a878a2ae32290045fb90cba9a44451e35341a36ef2ec1169d13592393aa4a8ca
| -rw-r--r-- | src/dbwrapper.cpp | 2 | ||||
| -rw-r--r-- | src/util/string.h | 8 | ||||
| -rw-r--r-- | src/wallet/wallet.cpp | 2 |
3 files changed, 10 insertions, 2 deletions
diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp index 775496e21b..479064d468 100644 --- a/src/dbwrapper.cpp +++ b/src/dbwrapper.cpp @@ -100,7 +100,7 @@ public: assert(p <= limit); base[std::min(bufsize - 1, (int)(p - base))] = '\0'; - LogPrintLevel(BCLog::LEVELDB, BCLog::Level::Debug, "%s", base); // NOLINT(bitcoin-unterminated-logprintf) + LogDebug(BCLog::LEVELDB, "%s\n", util::RemoveSuffixView(base, "\n")); if (base != buffer) { delete[] base; } diff --git a/src/util/string.h b/src/util/string.h index e2e470f4ab..30c0a0d6c1 100644 --- a/src/util/string.h +++ b/src/util/string.h @@ -81,6 +81,14 @@ std::vector<T> Split(const Span<const char>& sp, char sep) return std::string(TrimStringView(str, pattern)); } +[[nodiscard]] inline std::string_view RemoveSuffixView(std::string_view str, std::string_view suffix) +{ + if (str.ends_with(suffix)) { + return str.substr(0, str.size() - suffix.size()); + } + return str; +} + [[nodiscard]] inline std::string_view RemovePrefixView(std::string_view str, std::string_view prefix) { if (str.substr(0, prefix.size()) == prefix) { diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index bb1789f109..8e31950beb 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2309,7 +2309,7 @@ OutputType CWallet::TransactionChangeType(const std::optional<OutputType>& chang void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm) { LOCK(cs_wallet); - WalletLogPrintf("CommitTransaction:\n%s", tx->ToString()); // NOLINT(bitcoin-unterminated-logprintf) + WalletLogPrintf("CommitTransaction:\n%s\n", util::RemoveSuffixView(tx->ToString(), "\n")); // Add tx to wallet, because if it has change it's also ours, // otherwise just for transaction history. |