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.
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.
This passes the return value of _() directly to strprintf so the format
string can be checked at compile time in a future commit.
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>
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(
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31295.
See the guideline for information on the review process.
Type | Reviewers |
---|---|
ACK | ryanofsky, l0rinc, hodlinator |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
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"));
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}; }
?
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.
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)
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 👍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)cr-ACK fa3e074304780549b1e7972217930e34fa55f59a
0strprintf(_("Copyright (C) %i-%i").translated, 2009, COPYRIGHT_YEAR)
bilingual_str
of "Copyright (C) %i-%i"
.
G_TRANSLATION_FUN("Copyright (C) %i-%i")
, getting something like "Szerzői jog (c) %i-%i"
, and storing it in .translated
.strprintf()
(tfm::format(std::string...)
) on the returned .translated
and substitute in the values, yielding "Szerzői jog (c) 2009-2024"
.0strprintf(_("Copyright (C) %i-%i"), 2009, COPYRIGHT_YEAR).translated
bilingual_str
of "Copyright (C) %i-%i"
.
G_TRANSLATION_FUN("Copyright (C) %i-%i")
, getting something like "Szerzői jog (c) %i-%i"
, and storing it in .translated
.strprintf()
(tfm::format(bilingual_str...)
).
tfm::format
call for the .original
string.
args
type, and for bilingual_str
it fetches the .original
value for that arg.tfm::format
for the .translated
string.
args
type, and for bilingual_str
it fetches the .translated
value for that arg..translated
half of the strprintf
-result containing "Szerzői jog (c) 2009-2024"
(throwing away the work from 2.i, "Copyright (C) 2009-2024"
).End result is that we do slightly more work at runtime. Translation-strings themselves should not be affected.
This makes me disagree with the PR summary that the .translated
changes in faff8403f0aac3b5ec26d3c7fc98240f879f9906 “make sense on their own” without #31061. But I’m reasonably confident that PR will be merged after having reviewed it as well.