Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tinyformat: refactor: increase compile-time checks and don"t throw for tfm::format_error #30928

Closed
wants to merge 4 commits into from
Closed

tinyformat: refactor: increase compile-time checks and don"t throw for tfm::format_error #30928

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
104c805
move-only: ConstevalFormatString tfm::format to tinyformat.h
stickies-v Sep 25, 2024
4ce60d8
tinyformat: remove std::string overload
stickies-v Sep 25, 2024
4cf1221
tinyformat: force compile-time checks for format string literals
stickies-v Sep 25, 2024
49f7307
tinyformat: don't throw format string errors
stickies-v Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
tinyformat: remove std::string overload 
All (non-test) format strings are known at compile-time, so we
don"t need the std::string overload. Remove it so users use the
safer util::ConstevalFormatString where possible, or the
bilingual_str overload where applicable.
  • Loading branch information
@stickies-v
stickies-v committed Oct 8, 2024
commit 4ce60d82a0c1e21ee7ecba69e4c8926720829429
4 changes: 3 additions & 1 deletion doc/developer-notes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -950,7 +950,9 @@ Strings and formatting

- Do not use it when passing strings to `tfm::format`, `strprintf`, `LogInfo`, `LogDebug`, etc.

- *Rationale*: This is redundant. Tinyformat handles strings.
- *Rationale*: Tinyformat handles string parameters. Format strings
should be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.
Comment on lines +953 to +955
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we could avoid mentioning compile-time twice:

Suggested change
- *Rationale*: Tinyformat handles string parameters. Format strings
should be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.
- *Rationale*: Tinyformat handles string parameters.
Using a string literal or `constexpr const char*` ensures
format strings are validated at compile time.

Copy link
Contributor Author

@stickies-v stickies-v Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don"t think this is an improvement, I find it less clear about the distinction between the compile-time format string and the run-time string parameters, so I"m going to leave this as is for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why it"s less clear than before, but it"s just a nit from my part anyway

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree the current message is confusing. The distinction between format string and parameters to the format string should ideally be made clearer, as well as the actual string type.

Suggested change
- *Rationale*: Tinyformat handles string parameters. Format strings
should be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.
- *Rationale*: Tinyformat handles `std::string` parameters. Format strings
on the other hand should be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used std::string at first too, but then it was inconsistent with the rest of the documentation, which I think is even more unclear. I"ll reconsider updating all std::string references in this block if I have to force-push again unless you think this is blocking?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Tinyformat handles string parameters. is too controversial, I think it can be removed in the follow-up that removes the linter.

Otherwise, if this is replaced with std::string, someone else is going to suggest to mention std::string_view as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing tinyformat to support string_view is definitely out of scope for this PR.

I would at least want to make it clearer that format strings are separate from other parameters, like this or with other words to that effect:

Suggested change
- *Rationale*: Tinyformat handles string parameters. Format strings
should be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.
- *Rationale*: Tinyformat handles string parameters. Format strings
on the other hand should be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.

Even though I decided to drop into l0rinc"s thread here, I independently found it unclear when doing a review-pass on it.

More precise suggestion, but not blocking:

Suggested change
- *Rationale*: Tinyformat handles string parameters. Format strings
should be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.
- *Rationale*: Tinyformat handles string parameters. Format strings
on the other hand should (except for translations) be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would at least want to make it clearer that format strings are separate from other parameters

I agree with that, and I"ll incorporate your first suggestion (I don"t agree with the translations bit) if I have to force-push, thanks!


- Do not use it to convert to `QString`. Use `QString::fromStdString()`.

Expand Down
12 changes: 7 additions & 5 deletions src/clientversion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ std::string FormatSubVersion(const std::string& name, int nClientVersion, const

std::string CopyrightHolders(const std::string& strPrefix)
{
const auto copyright_devs = strprintf(_(COPYRIGHT_HOLDERS).translated, COPYRIGHT_HOLDERS_SUBSTITUTION);
const auto copyright_devs = strprintf(_(COPYRIGHT_HOLDERS), COPYRIGHT_HOLDERS_SUBSTITUTION).translated;
std::string strCopyrightHolders = strPrefix + copyright_devs;

// Make sure Bitcoin Core copyright is not removed by accident
Expand All @@ -85,15 +85,17 @@ std::string LicenseInfo()
{
const std::string URL_SOURCE_CODE = "<https://github.com/bitcoin/bitcoin>";

return CopyrightHolders(strprintf(_("Copyright (C) %i-%i").translated, 2009, COPYRIGHT_YEAR) + " ") + "\n" +
return CopyrightHolders(strprintf(_("Copyright (C) %i-%i"), 2009, COPYRIGHT_YEAR).translated + " ") + "\n" +
stickies-v marked this conversation as resolved.
Show resolved Hide resolved
"\n" +
strprintf(_("Please contribute if you find %s useful. "
"Visit %s for further information about the software.").translated, PACKAGE_NAME, "<" PACKAGE_URL ">") +
"Visit %s for further information about the software."),
PACKAGE_NAME, "<" PACKAGE_URL ">")
.translated +
"\n" +
strprintf(_("The source code is available from %s.").translated, URL_SOURCE_CODE) +
strprintf(_("The source code is available from %s."), URL_SOURCE_CODE).translated +
"\n" +
"\n" +
_("This is experimental software.").translated + "\n" +
strprintf(_("Distributed under the MIT software license, see the accompanying file %s or %s").translated, "COPYING", "<https://opensource.org/licenses/MIT>") +
strprintf(_("Distributed under the MIT software license, see the accompanying file %s or %s"), "COPYING", "<https://opensource.org/licenses/MIT>").translated +
"\n";
}
2 changes: 1 addition & 1 deletion src/init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,7 +617,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
strprintf("Maximum tip age in seconds to consider node in initial block download (default: %u)",
Ticks<std::chrono::seconds>(DEFAULT_MAX_TIP_AGE)),
ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
argsman.AddArg("-printpriority", strprintf("Log transaction fee rate in " + CURRENCY_UNIT + "/kvB when mining blocks (default: %u)", DEFAULT_PRINT_MODIFIED_FEE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
argsman.AddArg("-printpriority", strprintf("Log transaction fee rate in %s/kvB when mining blocks (default: %u)", CURRENCY_UNIT, DEFAULT_PRINT_MODIFIED_FEE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
argsman.AddArg("-uacomment=<cmt>", "Append comment to the user agent string", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);

SetupChainParamsBaseOptions(argsman);
Expand Down
28 changes: 14 additions & 14 deletions src/test/fuzz/strprintf.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,27 +52,27 @@ FUZZ_TARGET(str_printf)
CallOneOf(
fuzzed_data_provider,
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeRandomLengthString(32));
(void)strprintf(format_string.c_str(), fuzzed_data_provider.ConsumeRandomLengthString(32));
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeRandomLengthString(32));
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeRandomLengthString(32).c_str());
(void)strprintf(format_string.c_str(), fuzzed_data_provider.ConsumeRandomLengthString(32).c_str());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeRandomLengthString(32).c_str());
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<signed char>());
(void)strprintf(format_string.c_str(), fuzzed_data_provider.ConsumeIntegral<signed char>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<signed char>());
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<unsigned char>());
(void)strprintf(format_string.c_str(), fuzzed_data_provider.ConsumeIntegral<unsigned char>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<unsigned char>());
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<char>());
(void)strprintf(format_string.c_str(), fuzzed_data_provider.ConsumeIntegral<char>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<char>());
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeBool());
(void)strprintf(format_string.c_str(), fuzzed_data_provider.ConsumeBool());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeBool());
});
} catch (const tinyformat::format_error&) {
Expand All @@ -98,35 +98,35 @@ FUZZ_TARGET(str_printf)
CallOneOf(
fuzzed_data_provider,
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeFloatingPoint<float>());
(void)strprintf(format_string.c_str(), fuzzed_data_provider.ConsumeFloatingPoint<float>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeFloatingPoint<float>());
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeFloatingPoint<double>());
(void)strprintf(format_string.c_str(), fuzzed_data_provider.ConsumeFloatingPoint<double>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeFloatingPoint<double>());
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<int16_t>());
(void)strprintf(format_string.c_str(), fuzzed_data_provider.ConsumeIntegral<int16_t>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<int16_t>());
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<uint16_t>());
(void)strprintf(format_string.c_str(), fuzzed_data_provider.ConsumeIntegral<uint16_t>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<uint16_t>());
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<int32_t>());
(void)strprintf(format_string.c_str(), fuzzed_data_provider.ConsumeIntegral<int32_t>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<int32_t>());
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<uint32_t>());
(void)strprintf(format_string.c_str(), fuzzed_data_provider.ConsumeIntegral<uint32_t>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<uint32_t>());
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<int64_t>());
(void)strprintf(format_string.c_str(), fuzzed_data_provider.ConsumeIntegral<int64_t>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<int64_t>());
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<uint64_t>());
(void)strprintf(format_string.c_str(), fuzzed_data_provider.ConsumeIntegral<uint64_t>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<uint64_t>());
});
} catch (const tinyformat::format_error&) {
Expand Down
8 changes: 4 additions & 4 deletions src/test/txrequest_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,10 +178,10 @@ class Scenario
size_t real_total = runner.txrequest.Count(peer);
size_t real_candidates = runner.txrequest.CountCandidates(peer);
size_t real_inflight = runner.txrequest.CountInFlight(peer);
BOOST_CHECK_MESSAGE(real_total == total, strprintf("[" + comment + "] total %i (%i expected)", real_total, total));
stickies-v marked this conversation as resolved.
Show resolved Hide resolved
BOOST_CHECK_MESSAGE(real_inflight == inflight, strprintf("[" + comment + "] inflight %i (%i expected)", real_inflight, inflight));
BOOST_CHECK_MESSAGE(real_candidates == candidates, strprintf("[" + comment + "] candidates %i (%i expected)", real_candidates, candidates));
BOOST_CHECK_MESSAGE(ret == expected, "[" + comment + "] mismatching requestables");
BOOST_CHECK_MESSAGE(real_total == total, strprintf("[%s] total %i (%i expected)", comment, real_total, total));
BOOST_CHECK_MESSAGE(real_inflight == inflight, strprintf("[%s] inflight %i (%i expected)", comment, real_inflight, inflight));
BOOST_CHECK_MESSAGE(real_candidates == candidates, strprintf("[%s] candidates %i (%i expected)", comment, real_candidates, candidates));
BOOST_CHECK_MESSAGE(ret == expected, strprintf("[%s] mismatching requestables", comment));
});
}

Expand Down
7 changes: 0 additions & 7 deletions src/tinyformat.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1147,13 +1147,6 @@ TINYFORMAT_FOREACH_ARGNUM(TINYFORMAT_MAKE_FORMAT_FUNCS)
#endif

// Added for Bitcoin Core
template<typename... Args>
std::string format(const std::string &fmt, const Args&... args)
{
std::ostringstream oss;
format(oss, fmt.c_str(), args...);
return oss.str();
}
template <typename... Args>
std::string format(util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args)
{
Expand Down
4 changes: 2 additions & 2 deletions src/util/translation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ bilingual_str format(const bilingual_str& fmt, const Args&... args)
return arg;
}
}};
return bilingual_str{tfm::format(fmt.original, translate_arg(args, false)...),
tfm::format(fmt.translated, translate_arg(args, true)...)};
return bilingual_str{tfm::format(fmt.original.c_str(), translate_arg(args, false)...),
tfm::format(fmt.translated.c_str(), translate_arg(args, true)...)};
}
} // namespace tinyformat

Expand Down
2 changes: 1 addition & 1 deletion src/wallet/rpc/backup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,7 @@ RPCHelpMan importwallet()

// Use uiInterface.ShowProgress instead of pwallet.ShowProgress because pwallet.ShowProgress has a cancel button tied to AbortRescan which
// we don't want for this progress bar showing the import progress. uiInterface.ShowProgress does not have a cancel button.
pwallet->chain().showProgress(strprintf("%s " + _("Importing…").translated, pwallet->GetDisplayName()), 0, false); // show progress dialog in GUI
pwallet->chain().showProgress(strprintf("%s %s", pwallet->GetDisplayName(), _("Importing…").translated), 0, false); // show progress dialog in GUI
std::vector<std::tuple<CKey, int64_t, bool, std::string>> keys;
std::vector<std::pair<CScript, int64_t>> scripts;
while (file.good()) {
Expand Down
2 changes: 1 addition & 1 deletion src/wallet/test/fuzz/notifications.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ void ImportDescriptors(CWallet& wallet, const std::string& seed_insecure)

for (const std::string& desc_fmt : DESCS) {
for (bool internal : {true, false}) {
const auto descriptor{(strprintf)(desc_fmt, "[5aa9973a/66h/4h/2h]" + seed_insecure, int{internal})};
const auto descriptor{(strprintf)(desc_fmt.c_str(), "[5aa9973a/66h/4h/2h]" + seed_insecure, int{internal})};

FlatSigningProvider keys;
std::string error;
Expand Down
6 changes: 3 additions & 3 deletions src/wallet/wallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1891,7 +1891,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
fast_rescan_filter ? "fast variant using block filters" : "slow variant inspecting all blocks");

fAbortRescan = false;
ShowProgress(strprintf("%s " + _("Rescanning…").translated, GetDisplayName()), 0); // show rescan progress in GUI as dialog or on splashscreen, if rescan required on startup (e.g. due to corruption)
ShowProgress(strprintf("%s %s", GetDisplayName(), _("Rescanning…").translated), 0); // show rescan progress in GUI as dialog or on splashscreen, if rescan required on startup (e.g. due to corruption)
uint256 tip_hash = WITH_LOCK(cs_wallet, return GetLastBlockHash());
uint256 end_hash = tip_hash;
if (max_height) chain().findAncestorByHeight(tip_hash, *max_height, FoundBlock().hash(end_hash));
Expand All @@ -1906,7 +1906,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
m_scanning_progress = 0;
}
if (block_height % 100 == 0 && progress_end - progress_begin > 0.0) {
ShowProgress(strprintf("%s " + _("Rescanning…").translated, GetDisplayName()), std::max(1, std::min(99, (int)(m_scanning_progress * 100))));
ShowProgress(strprintf("%s %s", GetDisplayName(), _("Rescanning…").translated), std::max(1, std::min(99, (int)(m_scanning_progress * 100))));
}

bool next_interval = reserver.now() >= current_time + INTERVAL_TIME;
Expand Down Expand Up @@ -2003,7 +2003,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
WalletLogPrintf("Scanning current mempool transactions.\n");
WITH_LOCK(cs_wallet, chain().requestMempoolTransactions(*this));
}
ShowProgress(strprintf("%s " + _("Rescanning…").translated, GetDisplayName()), 100); // hide progress dialog in GUI
ShowProgress(strprintf("%s %s", GetDisplayName(), _("Rescanning…").translated), 100); // hide progress dialog in GUI
Copy link
Member

@maflcko maflcko Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit in 4ce60d8: Could split the changes to make the format string a bit more const, and never exclusively translated in real code into a separate commit? The benefit would be that the doc changes and (constroversial?) c_str changes are in a separate commit. It would also allow easier cherry-picking into other pulls.

Edit: Done in commit fa788db (and the previous commit). Feel free to rebase on them, or not, or otherwise take them, and modify them as you like.

if (block_height && fAbortRescan) {
WalletLogPrintf("Rescan aborted at block %d. Progress=%f\n", block_height, progress_current);
result.status = ScanResult::USER_ABORT;
Expand Down
2 changes: 1 addition & 1 deletion test/lint/run-lint-format-strings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
import sys

FALSE_POSITIVES = [
("src/clientversion.cpp", "strprintf(_(COPYRIGHT_HOLDERS).translated, COPYRIGHT_HOLDERS_SUBSTITUTION)"),
("src/clientversion.cpp", "strprintf(_(COPYRIGHT_HOLDERS), COPYRIGHT_HOLDERS_SUBSTITUTION)"),
("src/test/translation_tests.cpp", "strprintf(format, arg)"),
]

Expand Down