refactor: Prepare compile-time check of bilingual format strings

maflcko commented at 4:37 pm on November 15, 2024: member

The changes are required for #31061, however they also make sense on their own. For example, they are fixing up an inline namespace, which lead to compile errors otherwise (can be tested by observing the compile error after reverting the changes to src/util/strencodings.h). Also, a unit test comment is fixed.

refactor: Pick translated string after format

This passes the return value of _() directly to strprintf so the format
string can be checked at compile time in a future commit.

faff8403f0

move-only: Detail_CheckNumFormatSpecifiers and G_TRANSLATION_FUN

This is required for a future commit. Can be reviewed via the git
options --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

Also move util::detail::Hex to a proper namespace instead of an inline
namespace so it doesn't conflict with the new util::detail namespace, and
won't create other problems for callers trying to use the inline namespaces.

Also fix a misleading comment in util_string_tests.cpp.

Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>

fa72646f2b

refactor: Tidy fixups

Requested by clang-tidy:

src/wallet/salvage.cpp:119:18: error: use emplace_back instead of push_back [modernize-use-emplace,-warnings-as-errors]
   119 |         warnings.push_back(Untranslated("Salvage: Database salvage found errors, all data may not be recoverable."));
       |                  ^~~~~~~~~~
       |                  emplace_back(

fa3e074304

DrahtBot commented at 4:37 pm on November 15, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31295.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, l0rinc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
#26022 (Add util::ResultPtr class by ryanofsky)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Refactoring on Nov 15, 2024

ryanofsky approved

ryanofsky commented at 2:37 pm on November 18, 2024: contributor

Code review ACK fa3e074304780549b1e7972217930e34fa55f59a. Nice changes! These should allow related PRs to be simpler.

maflcko requested review from stickies-v on Nov 20, 2024

maflcko requested review from l0rinc on Nov 21, 2024

in src/wallet/feebumper.cpp:27 in fa3e074304

23@@ -24,24 +24,24 @@ namespace wallet {
24 static feebumper::Result PreconditionChecks(const CWallet& wallet, const CWalletTx& wtx, bool require_mine, std::vector<bilingual_str>& errors) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
25 {
26     if (wallet.HasWalletSpend(wtx.tx)) {
27-        errors.push_back(Untranslated("Transaction has descendants in the wallet"));
28+        errors.emplace_back(Untranslated("Transaction has descendants in the wallet"));

l0rinc commented at 10:44 am on November 21, 2024:

I tried reproducing the tidy warnings with

0$ clang++ --version
1Homebrew clang version 19.1.3
2
3$ cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
4&& cmake --build build -j$(nproc) \
5&& run-clang-tidy -p build -j$(nproc) -checks='-*,modernize-use-emplace' src/wallet/feebumper.cpp

But it doesn’t give me the mentioned warnings. The change itself does make sense (forwarding the bilingual_str params directly), I just can’t seem to reproduce the warning. Based on https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-emplace.html the check targets cases where a temporary object is explicitly constructed in the argument to push_back. Was this different in the original PR, where the construction happened inside of push_back (or related to any of the warnings below) or am I running something incorrectly?

Enabling all warnings for this file I do see a few other ones, some of them may be related to translations, e.g.

0bitcoin/src/wallet/feebumper.cpp:27:26: error: 'Untranslated' must resolve to a function declared within the namespace defined by the 'LIBC_NAMESPACE' macro [llvmlibc-callee-namespace,-warnings-as-errors]
1   27 |         errors.push_back(Untranslated("Transaction has descendants in the wallet"));
2      |                          ^
3bitcoin/src/util/translation.h:51:22: note: resolves to this declaration
4   51 | inline bilingual_str Untranslated(std::string original) { return {original, original}; }
5      |                      ^

and

0bitcoin/src/wallet/feebumper.cpp:49:26: error: 'format<std::string, std::string>' must resolve to a function declared within the namespace defined by the 'LIBC_NAMESPACE' macro [llvmlibc-callee-namespace,-warnings-as-errors]
1   49 |         errors.push_back(strprintf(Untranslated("Cannot bump transaction %s which was already bumped by transaction %s"), wtx.GetHash().ToString(), wtx.mapValue.at("replaced_by_txid")));
2      |                          ^
3bitcoin/src/tinyformat.h:1165:19: note: expanded from macro 'strprintf'
4 1165 | #define strprintf tfm::format
5      |                   ^
6bitcoin/src/util/translation.h:56:15: note: resolves to this declaration
7   56 | bilingual_str format(const bilingual_str& fmt, const Args&... args)
8      |               ^

nit, shouldn’t Untranslated pass by const reference instead (or nobody cares since it’s inline anyway), i.e.

0inline bilingual_str Untranslated(const std::string& original) { return {original, original}; }

?

maflcko commented at 12:04 pm on November 21, 2024:

I don’t think it matters either way in practise, because std::string can be moved so cheaply that an elided temporary won’t be noticed one way or another. This commit is just taken from the other pulls, so that clang-tidy is happy with them.

l0rinc commented at 12:08 pm on November 21, 2024:

Can you reproduce the warning in this PR? If not, what’s the reason for the warning in other PRs (and not here). If you can, what was I doing wrong?

maflcko commented at 12:34 pm on November 21, 2024:

It shouldn’t reproduce. This commit is just taken from the other pulls, so that clang-tidy is happy with them.

Edit: (resolving for now)

l0rinc approved

l0rinc commented at 11:57 am on November 21, 2024: contributor

ACK fa3e074304780549b1e7972217930e34fa55f59a

Lots of versions for this change, loosely coupled, but I understand they will simplify follow-ups. I have validated manually that:

run-lint-format-strings.py still needs the exception 👍
all code moves are same as before 👍
tidy warns for feebumper.cpp, salvage.cpp and wallet.cpp 👎 (couldn’t reproduce any of them, but manually checked it and at least it shouldn’t be worse than before)

refactor: Prepare compile-time check of bilingual format strings #31295

Code Coverage & Benchmarks

Reviews

Conflicts