Makes unequivocally clear for which type of format strings we do have parity between the two, and which we (currently) don’t.
Broken out from #30546 based on #30546 (review) and #30546 (review).
Makes unequivocally clear for which type of format strings we do have parity between the two, and which we (currently) don’t.
Broken out from #30546 based on #30546 (review) and #30546 (review).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process. A summary of reviews will appear here.
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.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30403435103
Make sure to run all tests locally, according to the documentation.
The failure may happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
18 {
19- // This was already executed at compile-time, but is executed again at run-time to avoid -Wunused.
20- decltype(fmt)::Detail_CheckNumFormatSpecifiers(fmt.fmt);
21+ // Prove parity with tinyformat
22+ switch (NumArgs) {
23+ case 0:
style nit (feel free to ignore): I suspect you could reduce verbosity and having to hand-format all of this by using tuple_cat
, see #30546 (review)
This should also ensure that all cases are covered and wouldn’t require to change the tests vectors.
Not sure it would reduce verbosity that much, but would definitely consider switching if someone took the time to provide a diff.
Compiler warnings are triggered for missing cases.
This should also ensure that all cases are covered and wouldn’t require to change the tests vectors.
If you don’t want to take it, that is fine. However, it would be good to at least terminate in the default case instead of silently passing. Otherwise it is unclear that all cases are covered.
While posting this PR I was aware of your tuple_cat
suggestion. I am not against it on principle but TBH I haven’t had a clear reason to struggle through that corner of C++ and don’t see your suggestion compelling enough to work through it. Your suggestion in that comment is close to compiling but doesn’t handle sending in one arg less for the failure cases. A motivation behind this PR is to prove parity in a clear way, and I’m worried it might obfuscate it. Happy to learn more from a diff.
Added default cases in dbbc2e52284a151a33d553b4a4032503906a4774, agree it’s more robust.
12@@ -13,17 +13,50 @@ BOOST_AUTO_TEST_SUITE(util_string_tests)
13 // Helper to allow compile-time sanity checks while providing the number of
14 // args directly. Normally PassFmt<sizeof...(Args)> would be used.
15 template <unsigned NumArgs>
16-inline void PassFmt(util::ConstevalFormatString<NumArgs> fmt)
17+inline void PassFmt(ConstevalFormatString<NumArgs> fmt)
18 {
19- // This was already executed at compile-time, but is executed again at run-time to avoid -Wunused.
20- decltype(fmt)::Detail_CheckNumFormatSpecifiers(fmt.fmt);
FailFmtWithError
, but happy path might only be run at compile time. Will bring it back.
14@@ -15,6 +15,12 @@
15 FALSE_POSITIVES = [
16 ("src/clientversion.cpp", "strprintf(_(COPYRIGHT_HOLDERS).translated, COPYRIGHT_HOLDERS_SUBSTITUTION)"),
17 ("src/test/translation_tests.cpp", "strprintf(format, arg)"),
18+ ("src/test/util_string_tests.cpp", "tfm::format(fmt.fmt)"),
19+ ("src/test/util_string_tests.cpp", 'tfm::format(fmt.fmt, "foo", "bar")'),
20+ ("src/test/util_string_tests.cpp", 'tfm::format(fmt.fmt, "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11")'),
21+ ("src/test/util_string_tests.cpp", 'tfm::format(fmt.fmt, "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12")'),
22+ ("src/test/util_string_tests.cpp", "tfm::format(fmt.fmt, a, b)"),
23+ ("src/test/util_string_tests.cpp", "tfm::format(fmt.fmt, a, b, c)"),
I’ll be happy to remove them once the linter is no longer run on CI.
That seems like extra churn and merge conflicts for no good reason. It should be trivial to avoid adding lines to this file. For example a simple #define tfm_fmt tfm::format
in the test (and using it) should avoid merge conflicts with future changes, or at least make them one-line conflicts at most.
Current approach:
Suggested approach:
#define
’s to temporarily work around a linter that is about to be removed, only to clean it up in another change seems like extra churn.Regardless:
tfm::*format*
function based on #30928.Maybe I’m missing something.
I’m not in a hurry to get this merged before other PRs in this area. Happy to move this to draft for now so other PRs can merge and deal with churn myself before undrafting.
Yes, exactly. Depending on how #30928 (review) turns out, this test-only pull will have to be adjusted.
There are already 4 different follow-ups, all of which explicitly or implicitly conflict with each other.
I don’t really want to open a 5th one to remove the linter first.
The more pull requests exist that explicitly or implicitly conflict with each other, the more review will be wasted, because if one of them is merged, all others will have to be rebased and re-reviewed. I understand that it isn’t always possible to avoid conflicts, but this whole changeset around formatting should be low priority enough to not require wasting (re)-review.
12@@ -13,17 +13,57 @@ BOOST_AUTO_TEST_SUITE(util_string_tests)
13 // Helper to allow compile-time sanity checks while providing the number of
14 // args directly. Normally PassFmt<sizeof...(Args)> would be used.
15 template <unsigned NumArgs>
16-inline void PassFmt(util::ConstevalFormatString<NumArgs> fmt)
17+inline void PassFmt(ConstevalFormatString<NumArgs> fmt)
18 {
19- // This was already executed at compile-time, but is executed again at run-time to avoid -Wunused.
20+ // Exercise happy paths at run-time for code coverage metrics.
I’m not sure I understand why we need this.
Why not make them all runtime instead, why are we “testing” compile time behavior here at all, aren’t we already doing that in the rest of the source code?
It seems to me the whole situation would simplify a lot if we would only test runtime behavior here.
constexpr
functions are not guaranteed to be able to evaluate at compile time for all possible parameters, so exercising that path in the test still provides some value IMO.
19- // This was already executed at compile-time, but is executed again at run-time to avoid -Wunused.
20+ // Exercise happy paths at run-time for code coverage metrics.
21 decltype(fmt)::Detail_CheckNumFormatSpecifiers(fmt.fmt);
22+
23+ // Prove parity with tinyformat
24+ switch (NumArgs) {
I like the idea of testing the validator together with the implementation!
But I’m not in love with the current approach.
Why are we testing PassFmt<1>("%02d");
with foo
- I don’t think it helps with understanding how formatter works.
Also PassFmt<1>("%s")
made sense when we were only validating the number of args, but we’ve extended it since, I think we should extend examples with the actual parameters, e.g.
PassFmt("%s", "test");
PassFmt("%02d", 42);
PassFmt("%12$s %2$s %1$s", "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12");
In which case each example would document the format that we’re exercising more specifically.
It would also obviate the parameter count by examples, would all be runtime (seems to me that would simplify a lot) and comparison would simply depend on the behavior of tfm::format
, e.g. validation would be something like:
0template <typename... Args>
1inline bool ValidateFmt(const char* fmt, const char* expected_error, const Args&... args)
2{
3 try {
4 tfm::format(fmt, args...);
5 ConstevalFormatString<sizeof...(Args)>::Detail_CheckNumFormatSpecifiers(fmt);
6 return true;
7 } catch (const tfm::format_error&) {
8 using ErrType = const char*;
9 auto check_throw = [expected_error](const ErrType& str) { return str == expected_error; };
10 BOOST_CHECK_EXCEPTION(ConstevalFormatString<sizeof...(Args)>::Detail_CheckNumFormatSpecifiers(fmt), ErrType, check_throw);
11 return false;
12 }
13}
14
15template <typename... Args>
16void PassFmt(const char* fmt, const Args&... args) { BOOST_CHECK(ValidateFmt(fmt, nullptr, args...)); }
17
18template <typename... Args>
19void FailFmt(const char* fmt, const char* expected_error, const Args&... args) { BOOST_CHECK(!ValidateFmt(fmt, expected_error, args...)); }
and usage would be something like
0 PassFmt("%%");
1 PassFmt("%s", "test");
2
3 auto err_num{"Format specifier count must match the argument count!"};
4 FailFmt("%s", err_num);
5 FailFmt("%s", err_num, "test", "extra");
tuple_cat
approach, leaving it feeling somewhat half-baked.
My aim is to retain the tests of leaving out one arg though (which your suggestion doesn’t include)
I didn’t include it since I though a single instance of those is enough in the tests.
I would prefer having concrete typed examples over retesting -1 and +1 args (for which explicit examples should likely suffice)
Co-Authored-By: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
30+
31+ // Prove parity with tinyformat
32+ TfmF(fmt.fmt, std::tuple_cat(std::array<int, NumArgs>{}));
33+ if constexpr (NumArgs > 0) {
34+ BOOST_CHECK_THROW(TfmF(fmt.fmt, std::tuple_cat(std::array<int, NumArgs - 1>{})), tfm::format_error);
35+ }
0 {
1 BOOST_CHECK_THROW(TfmF(fmt.fmt, std::tuple_cat(std::array<int, NumArgs + 1>{})), tfm::format_error);
2 }
style nit?
But let’s wait for CI first. There is a good chance that tuple_cat
doesn’t compile on Windows or one of the older compilers :sweat_smile:
Surely that’s more than style? :)
Would be nice to have both -1 and +1. Tried out your suggestion but tinyformat fails to throw in 8 of the cases. For example in "%12$s 999$s %2$s"
with 13 arguments, only using the 12th is not considered an error.
🐙 This pull request conflicts with the target branch and needs rebase.
Are you still working on this?
I am asking now, because there shouldn’t be any conflicts after rebase, I think.