Split out of #30928 (review)
The current string formatter couldn’t validate every string format template that we were using.
Extended it with dynamic widths, fixed a number parsing bug that could go over the string’s content and added a %n validation.
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.
No conflicts as of last run.
10@@ -11,75 +11,61 @@ using namespace util;
11
12 BOOST_AUTO_TEST_SUITE(util_string_tests)
13
14-// Helper to allow compile-time sanity checks while providing the number of
15-// args directly. Normally PassFmt<sizeof...(Args)> would be used.
16-template <unsigned NumArgs>
17-inline void PassFmt(util::ConstevalFormatString<NumArgs> fmt)
in cba51f2e380f0f89f799369b489c2e25e7215221: Not sure about turning the passing one into a runtime check from a compile-time check. Previously it was trivial to compile a single unit (this test) to sanity check the parser, as well as check the compile failures for various errors, by simply looking at the compile output. Also, the code was as close as possible to the real code, serving as documentation of how to use it.
Now, checking the passing cases requires not only compiling, but also linking and executing the test. Also, triggering a compile error to see that it works and to see how it looks is harder.
I understand you want better error messages on failure, but changing the passing cases isn’t required for that.
What is the benefit of ValidFormatSpecifiers over the existing PassFmt, other than dropping the code coverage stats?
Seems fine to update the comment below to say // Execute compile-time check again at run-time to get code coverage stats., but not sure about dropping it.
PassFmt I found the ValidFormatSpecifiers to be more specific (I’m not a fan of abbrvs and // comments).
Don’t have strong preference here, I can be convinced to rename it back, if you do.
Compared to
PassFmtI found theValidFormatSpecifiersto be more specific (I’m not a fan of abbrvs and // comments).
I don’t care about naming, so if you want to rename PassFmt to something else, this is fine. However, the // comment isn’t useless: It explains that the goal of this helper function is to be close as possible to the real code (and document the only difference). I found that useful as a single compilation unit that serves as a close proxy to the real code, with almost the same compile-time error messages (and behavior).
I understand you want better error messages on failure, but changing the behavior of the passing cases isn’t required for that.
Generally, if there isn’t a reason to change something, it is better to leave the code as-is, because it was most likely intentionally written in that way.
113+ if (count_normal && count_pos) throw "Format specifiers must be all positional or all non-positional!";
114+ int count{count_normal | count_pos};
115+ if (num_params != count) throw "Format specifier count must match the argument count!";
116+}
117+
118+template <int num_params>
-1 for test code, you could set constexpr INVALID=-1; and use that.
24@@ -25,58 +25,58 @@ namespace util {
25 * strings, to reduce the likelihood of tinyformat throwing exceptions at
26 * run-time. Validation is partial to try and prevent the most common errors
27 * while avoiding re-implementing the entire parsing logic.
28- *
29- * @note Counting of `*` dynamic width and precision fields (such as `%*c`,
30- * `%2$*3$d`, `%.*f`) is not implemented to minimize code complexity as long as
31- * they are not used in the codebase. Usage of these fields is not counted and
32- * can lead to run-time exceptions. Code wanting to use the `*` specifier can
33- * side-step this struct and call tinyformat directly.
format("'%1$*3$s %2$-*3$s'", "hi", "w", 12) is still unsupported and parsed incorrectly at compile time.
%*c which is a compile-time failure now).
* Renamed `Detail_CheckNumFormatSpecifiers` to `CheckFormatSpecifiers` since we're checking more than the number of parameters
* Moved it out of `ConstevalFormatString` to make it easier to test
* Inline `FailFmtWithError` (and rename `PassFmt` to `ValidFormatSpecifiers`) in tests to provide better errors messages on failure (e.g. line number)
They were used in bitcoin-cli
It's not supported in tinyformat: https://github.com/bitcoin/bitcoin/blob/master/src/tinyformat.h#L843-L845
25@@ -26,57 +26,63 @@ namespace util {
26 * run-time. Validation is partial to try and prevent the most common errors
27 * while avoiding re-implementing the entire parsing logic.
28 *
29- * @note Counting of `*` dynamic width and precision fields (such as `%*c`,
30+ * @note Counting of `*` dynamic width and precision fields (such as
31 * `%2$*3$d`, `%.*f`) is not implemented to minimize code complexity as long as
32 * they are not used in the codebase. Usage of these fields is not counted and
not sure about implementing a random and specific subset of * in specifiers. I think it is easier to either fully support them, or not at all. But having developers read the parser to understand which subset they are allowed to use may be causing more frustration than solving any real issue.
not implemented to minimize code complexity as long as they are not used in the codebase
I’ve implemented the part that was used, not a “random subset”
31 * `%2$*3$d`, `%.*f`) is not implemented to minimize code complexity as long as
32 * they are not used in the codebase. Usage of these fields is not counted and
33 * can lead to run-time exceptions. Code wanting to use the `*` specifier can
34 * side-step this struct and call tinyformat directly.
35 */
36+constexpr static void CheckFormatSpecifiers(std::string_view str, unsigned num_params)
Not sure about moving this out. This will break the doxygen comment above. Also, it drops the “detail-namespace”.
Generally, I think that test-only code should follow the real code, not the other way round. As long as real code is testable, optimizing other parts of the unit tests doesn’t seem too useful, especially if it breaks the existing construct and documentation.