From 275ab09ed7f9776190143fb6ff5cb6b87e0407cb Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 22 Nov 2022 19:42:43 +0100 Subject: [PATCH] Switch to a simpler wxString::Printf() implementation Don't use std::index_sequence, as this has some advantages: - This way doesn't require using another helper function, which is burdensome here and would be even more so if we had to do it for all the other wx vararg functions too. - We don't need to require C++14 and can keep supporting even g++ 4.8. The sole drawback is that we cannot pass the index of the argument to wxArgNormalizer ctor any longer, which means that we can't validate it, but this is addressed by validating the entire format string at once in the new Validate() member function, which is also more efficient than the old way for the format strings with more than one format specifier, as they only need to be parsed once now, instead of having to do it for each format specifier separately when GetArgumentType() is called. This also means that we can't handle char arguments differently depending on whether they are used with "%c" or "%d" format specifier. This is a backwards-incompatible change, but should affect very few use cases, so it seems to be worth breaking it to get the above benefits. --- include/wx/string.h | 45 +++++++----------------- include/wx/strvararg.h | 35 +++++++++---------- src/common/strvararg.cpp | 74 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 103 insertions(+), 51 deletions(-) diff --git a/include/wx/string.h b/include/wx/string.h index a8abf00549..dd77f7c326 100644 --- a/include/wx/string.h +++ b/include/wx/string.h @@ -43,7 +43,6 @@ #include "wx/beforestd.h" #include -#include #include #include "wx/afterstd.h" @@ -2198,13 +2197,20 @@ public: // formatted input/output // as sprintf(), returns the number of characters written or < 0 on error template - int Printf(const wxString& format, Targs... args) + int Printf(const wxFormatString& format, Targs... args) { - // Package the arguments into a tuple to pass them on together with the - // indices that are needed by the implementation. - return DoPrintf(format, - std::make_tuple(args...), - std::index_sequence_for{}); + format.Validate({wxFormatStringSpecifier::value...}); + +#if wxUSE_UNICODE_UTF8 + #if !wxUSE_UTF8_LOCALE_ONLY + if ( wxLocaleIsUtf8 ) + #endif + return DoPrintfUtf8(format, wxArgNormalizerUtf8{args, nullptr, 0}.get()...); +#endif // wxUSE_UNICODE_UTF8 + +#if !wxUSE_UTF8_LOCALE_ONLY + return DoPrintfWchar(format, wxArgNormalizerWchar{args, nullptr, 0}.get()...); +#endif // !wxUSE_UTF8_LOCALE_ONLY } // as vprintf(), returns the number of characters written or < 0 on error @@ -3386,31 +3392,6 @@ public: wxString& operator+=(wchar_t ch) { return *this += wxUniChar(ch); } private: - template - int DoPrintf(const wxFormatString& format, - const ArgsTuple& args, - std::index_sequence) - { -#if wxUSE_UNICODE_UTF8 - #if !wxUSE_UTF8_LOCALE_ONLY - if ( wxLocaleIsUtf8 ) - #endif - return DoPrintfUtf8 - ( - format, - wxNormalizeArgUtf8(std::get(args), &format, Ns).get()... - ); -#endif // wxUSE_UNICODE_UTF8 - -#if !wxUSE_UTF8_LOCALE_ONLY - return DoPrintfWchar - ( - format, - wxNormalizeArgWchar(std::get(args), &format, Ns).get()... - ); -#endif // !wxUSE_UTF8_LOCALE_ONLY - } - #if !wxUSE_UTF8_LOCALE_ONLY int DoPrintfWchar(const wxChar *format, ...); #endif diff --git a/include/wx/strvararg.h b/include/wx/strvararg.h index ccff218fcd..461bef874a 100644 --- a/include/wx/strvararg.h +++ b/include/wx/strvararg.h @@ -20,6 +20,7 @@ #include #include +#include #ifdef __cpp_lib_string_view #include @@ -195,6 +196,10 @@ public: Arg_Unknown = 0x8000 // unrecognized specifier (likely error) }; + // Validate all format string parameters at once: the vector contains the + // format specifiers corresponding to the actually given arguments. + void Validate(const std::vector& argTypes) const; + // returns the type of format specifier for n-th variadic argument (this is // not necessarily n-th format specifier if positional specifiers are used); // called by wxArgNormalizer<> specializations to get information about @@ -335,6 +340,13 @@ struct wxFormatStringArgumentFinder #define wxASSERT_ARG_TYPE(fmt, index, expected_mask) \ wxUnusedVar(fmt); \ wxUnusedVar(index) + + // Also provide a trivial implementation of Validate() doing nothing in + // this case. + inline void + wxFormatString::Validate(const std::vector& WXUNUSED(argTypes)) const + { + } #endif // wxDEBUG_LEVEL/!wxDEBUG_LEVEL @@ -459,6 +471,10 @@ struct wxArgNormalizer // to printf-like format string or nullptr if the variadic function doesn't // use format string and 'index' is index of 'value' in variadic arguments // list (starting at 1) + // + // Because the format string and index are used for checking for the format + // specifier mismatches and can be nullptr and 0, respectively, if they had + // been already checked using wxFormatString::Validate(). wxArgNormalizer(T value, const wxFormatString *fmt, unsigned index) : m_value(value) @@ -485,17 +501,6 @@ struct wxArgNormalizerWchar : public wxArgNormalizer : wxArgNormalizer(value, fmt, index) {} }; -// Helper function for creating a wxArgNormalizerWchar with type deduced -// from the type of the argument. -// -// NB: Unlike the class ctor, which uses 1-based indices, it takes 0-based -// index which makes it more convenient to use in pack expansions. -template -wxArgNormalizerWchar -wxNormalizeArgWchar(T value, const wxFormatString *fmt, unsigned indexFrom0) -{ - return wxArgNormalizerWchar(value, fmt, indexFrom0 + 1); -} #endif // !wxUSE_UTF8_LOCALE_ONLY // normalizer for passing arguments to functions working with UTF-8 encoded @@ -509,14 +514,6 @@ wxNormalizeArgWchar(T value, const wxFormatString *fmt, unsigned indexFrom0) : wxArgNormalizer(value, fmt, index) {} }; - // Same type-deducing helper as above, but for wxArgNormalizerUtf8 - template - wxArgNormalizerUtf8 - wxNormalizeArgUtf8(T value, const wxFormatString *fmt, unsigned indexFrom0) - { - return wxArgNormalizerUtf8(value, fmt, indexFrom0 + 1); - } - #define wxArgNormalizerNative wxArgNormalizerUtf8 #else // wxUSE_UNICODE_WCHAR #define wxArgNormalizerNative wxArgNormalizerWchar diff --git a/src/common/strvararg.cpp b/src/common/strvararg.cpp index 99adde4f11..dafdd2cf74 100644 --- a/src/common/strvararg.cpp +++ b/src/common/strvararg.cpp @@ -732,6 +732,64 @@ wxFormatString::ArgumentType DoGetArgumentType(const CharType *format, return ArgTypeFromParamType(parser.pspec[n-1]->m_type); } +#if wxDEBUG_LEVEL + +template +void DoValidateFormat(const CharType* format, const std::vector& argTypes) +{ + wxPrintfConvSpecParser parser(format); + + // For the reasons mentioned in the comment in DoGetArgumentType() above, + // we ignore any extraneous argument types, so we only check that the + // format format specifiers we actually have match the types. + for ( unsigned n = 0; n < parser.nargs; ++n ) + { + if ( n == argTypes.size() ) + { + wxFAIL_MSG + ( + wxString::Format + ( + "Not enough arguments, %zu given but at least %u needed", + argTypes.size(), + parser.nargs + ) + ); + + // Useless to continue further. + return; + } + + auto const pspec = parser.pspec[n]; + if ( !pspec ) + { + wxFAIL_MSG + ( + wxString::Format + ( + "Missing format specifier for argument %u in \"%s\"", + n + 1, format + ) + ); + + continue; + } + + auto const ptype = ArgTypeFromParamType(pspec->m_type); + wxASSERT_MSG + ( + (ptype & argTypes[n]) == ptype, + wxString::Format + ( + "Format specifier mismatch for argument %u of \"%s\"", + n + 1, format + ) + ); + } +} + +#endif // wxDEBUG_LEVEL + } // anonymous namespace wxFormatString::ArgumentType wxFormatString::GetArgumentType(unsigned n) const @@ -748,3 +806,19 @@ wxFormatString::ArgumentType wxFormatString::GetArgumentType(unsigned n) const wxFAIL_MSG( "unreachable code" ); return Arg_Unknown; } + +#if wxDEBUG_LEVEL + +void wxFormatString::Validate(const std::vector& argTypes) const +{ + if ( m_char ) + DoValidateFormat(m_char.data(), argTypes); + else if ( m_wchar ) + DoValidateFormat(m_wchar.data(), argTypes); + else if ( m_str ) + DoValidateFormat(m_str->wx_str(), argTypes); + else if ( m_cstr ) + DoValidateFormat(m_cstr->AsInternal(), argTypes); +} + +#endif // wxDEBUG_LEVEL