refactor: Prepare compile-time check of bilingual format strings #31295
Conversation
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 <[email protected]>
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. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31295. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
There was a problem hiding this comment.
ACK fa3e074
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.pystill needs the exception 👍- all code moves are same as before 👍
- tidy warns for
feebumper.cpp,salvage.cppandwallet.cpp👎 (couldn't reproduce any of them, but manually checked it and at least it shouldn't be worse than before)
There was a problem hiding this comment.
cr-ACK fa3e074
Before
strprintf(_("Copyright (C) %i-%i").translated, 2009, COPYRIGHT_YEAR)- Create a
bilingual_strof"Copyright (C) %i-%i".- Calls
G_TRANSLATION_FUN("Copyright (C) %i-%i"), getting something like"Szerzői jog (c) %i-%i", and storing it in.translated.
- Calls
- Perform regular
strprintf()(tfm::format(std::string...)) on the returned.translatedand substitute in the values, yielding"Szerzői jog (c) 2009-2024".
After
strprintf(_("Copyright (C) %i-%i"), 2009, COPYRIGHT_YEAR).translated- Create a
bilingual_strof"Copyright (C) %i-%i".- Calls
G_TRANSLATION_FUN("Copyright (C) %i-%i"), getting something like"Szerzői jog (c) %i-%i", and storing it in.translated.
- Calls
- Perform special
strprintf()(tfm::format(bilingual_str...)).tfm::formatcall for the.originalstring.- Checks each
argstype, and forbilingual_strit fetches the.originalvalue for that arg.
- Checks each
tfm::formatfor the.translatedstring.- Checks each
argstype, and forbilingual_strit fetches the.translatedvalue for that arg.
- Checks each
- Fetch the
.translatedhalf of thestrprintf-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 faff840 "make sense on their own" without #31061. But I'm reasonably confident that PR will be merged after having reviewed it as well.
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 tosrc/util/strencodings.h). Also, a unit test comment is fixed.