From 769e887cf162954c9378fd69893e8e54a0cbe06e Mon Sep 17 00:00:00 2001 From: Aras Pranckevicius Date: Tue, 1 Oct 2024 12:32:12 +0300 Subject: [PATCH 1/7] Iridas .cube and other text format parsing optimizations Primarily driven by a wish to increase performance of parsing .cube files. But the functions that are added or changed are used across parsing of all/most of text based formats. With these changes, parsing "Khronos PBR Neutral" .cube file (5.4MB) on Ryzen 5950X / VS2022 Release build: 167ms -> 123ms. - Add locale independent IsSpace(char). Somewhat similar to 3aab90d, whitespace trimming perhaps should not be locale dependent. - Add IsEmptyOrWhiteSpace() and use that inside ParseUtils::nextline, instead of doing Trim(line).empty(). - Add StringUtils::StartsWith(char) and use that in various parsers that were constructing whole std::string object just to check for a single character. - When building for C++17 or later, NumberUtils can use standard from_chars functions (except on Apple platforms, where those are not implemented for floating point types as of Xcode 15). This has advantage of not having to deal with errno or locales. Saves some thread local storage accesses and function calls (e.g. on Windows errno is actually a function call). - There's a CMake setup change that adds /Zc:__cplusplus flag for MSVC; for backwards compat reasons it does not report proper C++ version detection defines otherwise. Signed-off-by: Aras Pranckevicius --- share/cmake/utils/CompilerFlags.cmake | 3 ++ src/OpenColorIO/LookParse.cpp | 4 +- src/OpenColorIO/ParseUtils.cpp | 4 +- src/OpenColorIO/fileformats/FileFormat3DL.cpp | 4 +- .../fileformats/FileFormatIridasCube.cpp | 6 +-- .../fileformats/FileFormatIridasItx.cpp | 2 +- .../fileformats/FileFormatPandora.cpp | 2 +- .../fileformats/FileFormatResolveCube.cpp | 2 +- .../fileformats/FileFormatSpi1D.cpp | 2 +- .../fileformats/FileFormatTruelight.cpp | 2 +- src/OpenColorIO/fileformats/FileFormatVF.cpp | 2 +- src/utils/NumberUtils.h | 48 +++++++++++++++++-- src/utils/StringUtils.h | 24 +++++++++- .../FileFormatIridasCube_tests.cpp | 37 ++++++++++++++ 14 files changed, 122 insertions(+), 20 deletions(-) diff --git a/share/cmake/utils/CompilerFlags.cmake b/share/cmake/utils/CompilerFlags.cmake index 91e438f455..c1f6272480 100644 --- a/share/cmake/utils/CompilerFlags.cmake +++ b/share/cmake/utils/CompilerFlags.cmake @@ -62,6 +62,9 @@ if(USE_MSVC) ) endif() + # Make MSVC compiler report correct __cplusplus version (otherwise reports 199711L) + set(PLATFORM_COMPILE_OPTIONS "${PLATFORM_COMPILE_OPTIONS};/Zc:__cplusplus") + # Explicitely specify the default warning level i.e. /W3. # Note: Do not use /Wall (i.e. /W4) which adds 'informational' warnings. set(PLATFORM_COMPILE_OPTIONS "${PLATFORM_COMPILE_OPTIONS};/W3") diff --git a/src/OpenColorIO/LookParse.cpp b/src/OpenColorIO/LookParse.cpp index 2b83270e0d..e70a09cf4b 100644 --- a/src/OpenColorIO/LookParse.cpp +++ b/src/OpenColorIO/LookParse.cpp @@ -17,13 +17,13 @@ void LookParseResult::Token::parse(const std::string & str) { // Assert no commas, colons, or | in str. - if(StringUtils::StartsWith(str, "+")) + if(StringUtils::StartsWith(str, '+')) { name = StringUtils::LeftTrim(str, '+'); dir = TRANSFORM_DIR_FORWARD; } // TODO: Handle -- - else if(StringUtils::StartsWith(str, "-")) + else if(StringUtils::StartsWith(str, '-')) { name = StringUtils::LeftTrim(str, '-'); dir = TRANSFORM_DIR_INVERSE; diff --git a/src/OpenColorIO/ParseUtils.cpp b/src/OpenColorIO/ParseUtils.cpp index 9ec635bebb..e7f09c6d27 100644 --- a/src/OpenColorIO/ParseUtils.cpp +++ b/src/OpenColorIO/ParseUtils.cpp @@ -672,13 +672,13 @@ bool nextline(std::istream &istream, std::string &line) { line.resize(line.size() - 1); } - if(!StringUtils::Trim(line).empty()) + if(!StringUtils::IsEmptyOrWhiteSpace(line)) { return true; } } - line = ""; + line.clear(); return false; } diff --git a/src/OpenColorIO/fileformats/FileFormat3DL.cpp b/src/OpenColorIO/fileformats/FileFormat3DL.cpp index 0babf55206..c4d90cc776 100755 --- a/src/OpenColorIO/fileformats/FileFormat3DL.cpp +++ b/src/OpenColorIO/fileformats/FileFormat3DL.cpp @@ -263,11 +263,11 @@ CachedFileRcPtr LocalFileFormat::read(std::istream & istream, if(lineParts.empty()) continue; if (lineParts.size() > 0) { - if (StringUtils::StartsWith(lineParts[0], "#")) + if (StringUtils::StartsWith(lineParts[0], '#')) { continue; } - if (StringUtils::StartsWith(lineParts[0], "<")) + if (StringUtils::StartsWith(lineParts[0], '<')) { // Format error: reject files that could be // formatted as xml. diff --git a/src/OpenColorIO/fileformats/FileFormatIridasCube.cpp b/src/OpenColorIO/fileformats/FileFormatIridasCube.cpp index aa00f7febc..ca2965e373 100755 --- a/src/OpenColorIO/fileformats/FileFormatIridasCube.cpp +++ b/src/OpenColorIO/fileformats/FileFormatIridasCube.cpp @@ -180,7 +180,7 @@ LocalFileFormat::read(std::istream & istream, { ++lineNumber; // All lines starting with '#' are comments - if (StringUtils::StartsWith(line,"#")) continue; + if (StringUtils::StartsWith(line,'#')) continue; line = StringUtils::Lower(StringUtils::Trim(line)); @@ -310,10 +310,10 @@ LocalFileFormat::read(std::istream & istream, do { - line = StringUtils::Trim(line); + line = StringUtils::LeftTrim(line); // All lines starting with '#' are comments - if (StringUtils::StartsWith(line,"#")) continue; + if (StringUtils::StartsWith(line,'#')) continue; if (line.empty()) continue; diff --git a/src/OpenColorIO/fileformats/FileFormatIridasItx.cpp b/src/OpenColorIO/fileformats/FileFormatIridasItx.cpp index be903d5d33..ae6282efb8 100755 --- a/src/OpenColorIO/fileformats/FileFormatIridasItx.cpp +++ b/src/OpenColorIO/fileformats/FileFormatIridasItx.cpp @@ -146,7 +146,7 @@ CachedFileRcPtr LocalFileFormat::read(std::istream & istream, { ++lineNumber; // All lines starting with '#' are comments - if(StringUtils::StartsWith(line,"#")) continue; + if(StringUtils::StartsWith(line,'#')) continue; // Strip, lowercase, and split the line parts = StringUtils::SplitByWhiteSpaces(StringUtils::Lower(StringUtils::Trim(line))); diff --git a/src/OpenColorIO/fileformats/FileFormatPandora.cpp b/src/OpenColorIO/fileformats/FileFormatPandora.cpp index 5018de6ac3..6d36321527 100755 --- a/src/OpenColorIO/fileformats/FileFormatPandora.cpp +++ b/src/OpenColorIO/fileformats/FileFormatPandora.cpp @@ -129,7 +129,7 @@ CachedFileRcPtr LocalFileFormat::read(std::istream & istream, if(parts.empty()) continue; // Skip all lines starting with '#' - if(StringUtils::StartsWith(parts[0],"#")) continue; + if(StringUtils::StartsWith(parts[0],'#')) continue; if(parts[0] == "channel") { diff --git a/src/OpenColorIO/fileformats/FileFormatResolveCube.cpp b/src/OpenColorIO/fileformats/FileFormatResolveCube.cpp index 9c5e23180e..6b5e0da8d7 100755 --- a/src/OpenColorIO/fileformats/FileFormatResolveCube.cpp +++ b/src/OpenColorIO/fileformats/FileFormatResolveCube.cpp @@ -296,7 +296,7 @@ CachedFileRcPtr LocalFileFormat::read(std::istream & istream, ++lineNumber; // All lines starting with '#' are comments - if(StringUtils::StartsWith(line,"#")) + if(StringUtils::StartsWith(line,'#')) { if(headerComplete) { diff --git a/src/OpenColorIO/fileformats/FileFormatSpi1D.cpp b/src/OpenColorIO/fileformats/FileFormatSpi1D.cpp index e2e4822b45..1cdc51b22c 100755 --- a/src/OpenColorIO/fileformats/FileFormatSpi1D.cpp +++ b/src/OpenColorIO/fileformats/FileFormatSpi1D.cpp @@ -168,7 +168,7 @@ CachedFileRcPtr LocalFileFormat::read(std::istream & istream, } } } - while (istream.good() && !StringUtils::StartsWith(headerLine,"{")); + while (istream.good() && !StringUtils::StartsWith(headerLine,'{')); } if (version == -1) diff --git a/src/OpenColorIO/fileformats/FileFormatTruelight.cpp b/src/OpenColorIO/fileformats/FileFormatTruelight.cpp index e3ee605026..0d95261cc3 100755 --- a/src/OpenColorIO/fileformats/FileFormatTruelight.cpp +++ b/src/OpenColorIO/fileformats/FileFormatTruelight.cpp @@ -124,7 +124,7 @@ CachedFileRcPtr LocalFileFormat::read(std::istream & istream, if(parts.empty()) continue; // Parse header metadata (which starts with #) - if(StringUtils::StartsWith(parts[0],"#")) + if(StringUtils::StartsWith(parts[0],'#')) { if(parts.size() < 2) continue; diff --git a/src/OpenColorIO/fileformats/FileFormatVF.cpp b/src/OpenColorIO/fileformats/FileFormatVF.cpp index 03ff191d5f..9026030344 100755 --- a/src/OpenColorIO/fileformats/FileFormatVF.cpp +++ b/src/OpenColorIO/fileformats/FileFormatVF.cpp @@ -129,7 +129,7 @@ CachedFileRcPtr LocalFileFormat::read(std::istream & istream, if(parts.empty()) continue; - if(StringUtils::StartsWith(parts[0],"#")) continue; + if(StringUtils::StartsWith(parts[0],'#')) continue; if(!in3d) { diff --git a/src/utils/NumberUtils.h b/src/utils/NumberUtils.h index f6835a0a16..05231c4d2a 100644 --- a/src/utils/NumberUtils.h +++ b/src/utils/NumberUtils.h @@ -4,6 +4,18 @@ #ifndef INCLUDED_NUMBERUTILS_H #define INCLUDED_NUMBERUTILS_H +// With C++17, we can use from_chars. +// +// That has advantage of not dealing with locale / errno, +// which might involve locks or thread local storage accesses. +// +// Except on Apple platforms, where (as of Xcode 15 / Apple Clang 15) +// these are not implemented for float/double types. +#if __cpp_lib_to_chars >= 201611L && !defined(__APPLE__) +#define USE_CHARCONV_FROM_CHARS +#include +#endif + #if defined(_MSC_VER) #define really_inline __forceinline #else @@ -56,12 +68,21 @@ static const Locale loc; really_inline from_chars_result from_chars(const char *first, const char *last, double &value) noexcept { - errno = 0; if (!first || !last || first == last) { return {first, std::errc::invalid_argument}; } +#ifdef USE_CHARCONV_FROM_CHARS + if (first < last && first[0] == '+') + { + ++first; + } + std::from_chars_result res = std::from_chars(first, last, value); + return from_chars_result{ res.ptr, res.ec }; +#else + + errno = 0; char * endptr = nullptr; double @@ -88,16 +109,26 @@ really_inline from_chars_result from_chars(const char *first, const char *last, { return {first, std::errc::argument_out_of_domain}; } +#endif } really_inline from_chars_result from_chars(const char *first, const char *last, float &value) noexcept { - errno = 0; if (!first || !last || first == last) { return {first, std::errc::invalid_argument}; } +#ifdef USE_CHARCONV_FROM_CHARS + if (first < last && first[0] == '+') + { + ++first; + } + std::from_chars_result res = std::from_chars(first, last, value); + return from_chars_result{ res.ptr, res.ec }; +#else + + errno = 0; char *endptr = nullptr; float @@ -132,16 +163,26 @@ really_inline from_chars_result from_chars(const char *first, const char *last, { return {first, std::errc::argument_out_of_domain}; } +#endif } really_inline from_chars_result from_chars(const char *first, const char *last, long int &value) noexcept { - errno = 0; if (!first || !last || first == last) { return {first, std::errc::invalid_argument}; } +#ifdef USE_CHARCONV_FROM_CHARS + if (first < last && first[0] == '+') + { + ++first; + } + std::from_chars_result res = std::from_chars(first, last, value); + return from_chars_result{ res.ptr, res.ec }; +#else + + errno = 0; char *endptr = nullptr; long int @@ -170,6 +211,7 @@ really_inline from_chars_result from_chars(const char *first, const char *last, { return {first, std::errc::argument_out_of_domain}; } +#endif } } // namespace NumberUtils } // namespace OCIO_NAMESPACE diff --git a/src/utils/StringUtils.h b/src/utils/StringUtils.h index cc1cf4cd43..7ad0624581 100644 --- a/src/utils/StringUtils.h +++ b/src/utils/StringUtils.h @@ -44,6 +44,13 @@ inline unsigned char Upper(unsigned char c) } } +// Checks if character is whitespace (space, tab, newline or other +// non-printable char). Does not take locale into account. +inline bool IsSpace(char c) +{ + return c <= ' '; +} + // Return the lower case string. inline std::string Lower(std::string str) { @@ -95,6 +102,13 @@ inline bool StartsWith(const std::string & str, const std::string & prefix) return str.size() >= prefix.size() && 0 == str.compare(0, prefix.size(), prefix); } +// Return true if the string starts with the prefix character. +// Note: The comparison is case sensitive. +inline bool StartsWith(const std::string& str, char prefix) +{ + return !str.empty() && str.front() == prefix; +} + // Starting from the left, trim the character. inline std::string LeftTrim(std::string str, char c) { @@ -106,7 +120,7 @@ inline std::string LeftTrim(std::string str, char c) // Starting from the left, trim all the space characters i.e. space, tabulation, etc. inline std::string LeftTrim(std::string str) { - const auto it = std::find_if(str.begin(), str.end(), [](char ch) { return !std::isspace(static_cast(ch)); }); + const auto it = std::find_if(str.begin(), str.end(), [](char ch) { return !IsSpace(ch); }); str.erase(str.begin(), it); return str; } @@ -123,7 +137,7 @@ inline std::string RightTrim(std::string str, char c) inline std::string RightTrim(std::string str) { const auto it = - std::find_if(str.rbegin(), str.rend(), [](char ch) { return !std::isspace(static_cast(ch)); }); + std::find_if(str.rbegin(), str.rend(), [](char ch) { return !IsSpace(ch); }); str.erase(it.base(), str.end()); return str; } @@ -148,6 +162,12 @@ inline void Trim(StringVec & list) } } +// Checks if string is empty or white-space only. +inline bool IsEmptyOrWhiteSpace(const std::string& str) +{ + return std::find_if(str.begin(), str.end(), [](char ch) { return !IsSpace(ch); }) == str.end(); +} + // Split a string content using an arbitrary separator. inline StringVec Split(const std::string & str, char separator) { diff --git a/tests/cpu/fileformats/FileFormatIridasCube_tests.cpp b/tests/cpu/fileformats/FileFormatIridasCube_tests.cpp index 13312bdb5d..b834ce10a7 100644 --- a/tests/cpu/fileformats/FileFormatIridasCube_tests.cpp +++ b/tests/cpu/fileformats/FileFormatIridasCube_tests.cpp @@ -161,6 +161,43 @@ OCIO_ADD_TEST(FileFormatIridasCube, read_failure) } } +OCIO_ADD_TEST(FileFormatIridasCube, whitespace_handling) +{ + const std::string SAMPLE = + "# comment\n" + "# comment with trailing space \n" + "# next up various forms of empty lines\n" + "\n" + " \n" + " \t \n" + "# whitespace before keywords or after data should be supported\n" + " LUT_3D_SIZE \t 2 \t\n" + "\t \tDOMAIN_MIN 0.25 0.5 0.75\n" + "\n" + "DOMAIN_MAX\t1.5\t2.5\t3.5\n" + + "0.0 0.0 0.0\n" + "# comments in between data should be ignored\n" + " 1.0 0.0 \t 0.0\n" + "0.0 1.0 0.0 \n" + " 1.0 1.0 0.0\n" + " \n" + "0.0 0.0 1.0\n" + "1.0 0.0 1.0\n" + "0.0 1.0 1.0\n" + "1.0 1.0 1.0\n" + " \n" + "\n"; + + OCIO::LocalCachedFileRcPtr file; + OCIO_CHECK_NO_THROW(file = ReadIridasCube(SAMPLE)); + OCIO_CHECK_EQUAL(file->domain_min[0], 0.25f); + OCIO_CHECK_EQUAL(file->domain_min[2], 0.75f); + OCIO_CHECK_EQUAL(file->domain_max[0], 1.5f); + OCIO_CHECK_EQUAL(file->domain_max[2], 3.5f); + OCIO_CHECK_EQUAL(file->lut3D->getArray().getLength(), 2); +} + OCIO_ADD_TEST(FileFormatIridasCube, no_shaper) { // check baker output From 74b465aeaead21bbd00d666b51be72537b77c926 Mon Sep 17 00:00:00 2001 From: Aras Pranckevicius Date: Tue, 1 Oct 2024 19:54:46 +0300 Subject: [PATCH 2/7] Fix test failures (char can be signed, doh) Signed-off-by: Aras Pranckevicius --- src/utils/StringUtils.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/StringUtils.h b/src/utils/StringUtils.h index 7ad0624581..7dbfaa1a4b 100644 --- a/src/utils/StringUtils.h +++ b/src/utils/StringUtils.h @@ -46,7 +46,7 @@ inline unsigned char Upper(unsigned char c) // Checks if character is whitespace (space, tab, newline or other // non-printable char). Does not take locale into account. -inline bool IsSpace(char c) +inline bool IsSpace(unsigned char c) { return c <= ' '; } From 8f5be6ad6de01cc5bc8254b1947e8828448b5b07 Mon Sep 17 00:00:00 2001 From: Aras Pranckevicius Date: Wed, 2 Oct 2024 10:30:02 +0300 Subject: [PATCH 3/7] Tests: add unit test coverage for NumberUtils::from_chars directly Currently it was only tested indirectly via XMLReaderUtils_tests and file format tests Signed-off-by: Aras Pranckevicius --- tests/utils/CMakeLists.txt | 1 + tests/utils/NumberUtils_tests.cpp | 81 +++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+) create mode 100644 tests/utils/NumberUtils_tests.cpp diff --git a/tests/utils/CMakeLists.txt b/tests/utils/CMakeLists.txt index 242cad4f0d..bf1858ff92 100644 --- a/tests/utils/CMakeLists.txt +++ b/tests/utils/CMakeLists.txt @@ -5,6 +5,7 @@ include(ExternalProject) set(SOURCES UnitTestMain.cpp + NumberUtils_tests.cpp StringUtils_tests.cpp ) diff --git a/tests/utils/NumberUtils_tests.cpp b/tests/utils/NumberUtils_tests.cpp new file mode 100644 index 0000000000..faaeba334f --- /dev/null +++ b/tests/utils/NumberUtils_tests.cpp @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: BSD-3-Clause +// Copyright Contributors to the OpenColorIO Project. + + +#include "testutils/UnitTest.h" +#include "utils/NumberUtils.h" + +namespace OCIO = OCIO_NAMESPACE; + +OCIO_ADD_TEST(NumberUtils, from_chars_float) +{ +#define TEST_FROM_CHARS(text) \ + str = text; \ + res = OCIO::NumberUtils::from_chars(str.data(), str.data() + str.size(), val); \ + OCIO_CHECK_ASSERT(res.ec == std::errc()); \ + OCIO_CHECK_ASSERT(res.ptr == str.data() + str.size()) + + std::string str; + float val; + OCIO::NumberUtils::from_chars_result res; + + // regular numbers + TEST_FROM_CHARS("-7"); OCIO_CHECK_EQUAL(val, -7.0f); + TEST_FROM_CHARS("1.5"); OCIO_CHECK_EQUAL(val, 1.5f); + TEST_FROM_CHARS("-17.25"); OCIO_CHECK_EQUAL(val, -17.25f); + TEST_FROM_CHARS("-.75"); OCIO_CHECK_EQUAL(val, -.75f); + TEST_FROM_CHARS("11."); OCIO_CHECK_EQUAL(val, 11.0f); + // exponent notation + TEST_FROM_CHARS("1e3"); OCIO_CHECK_EQUAL(val, 1000.0f); + TEST_FROM_CHARS("1e+2"); OCIO_CHECK_EQUAL(val, 100.0f); + TEST_FROM_CHARS("50e-2"); OCIO_CHECK_EQUAL(val, 0.5f); + TEST_FROM_CHARS("-1.5e2"); OCIO_CHECK_EQUAL(val, -150.0f); + // whitespace/prefix handling + TEST_FROM_CHARS("+57.125"); OCIO_CHECK_EQUAL(val, +57.125f); + TEST_FROM_CHARS(" \t 123.5"); OCIO_CHECK_EQUAL(val, 123.5f); + // special values + TEST_FROM_CHARS("-infinity"); OCIO_CHECK_EQUAL(val, -std::numeric_limits::infinity()); + TEST_FROM_CHARS("nan"); OCIO_CHECK_ASSERT(std::isnan(val)); + // hex format should be parsed + TEST_FROM_CHARS("0x42"); OCIO_CHECK_EQUAL(val, 66.0f); + TEST_FROM_CHARS("0x42ab.c"); OCIO_CHECK_EQUAL(val, 17067.75f); + + // valid numbers with trailing non-number chars should stop there + str = "-7.5ab"; + res = OCIO::NumberUtils::from_chars(str.data(), str.data() + str.size(), val); + OCIO_CHECK_ASSERT(res.ptr == str.data() + 4); + OCIO_CHECK_EQUAL(val, -7.5f); + str = "infinitya"; + res = OCIO::NumberUtils::from_chars(str.data(), str.data() + str.size(), val); + OCIO_CHECK_ASSERT(res.ptr == str.data() + 8); + OCIO_CHECK_EQUAL(val, std::numeric_limits::infinity()); + str = "0x18g"; + res = OCIO::NumberUtils::from_chars(str.data(), str.data() + str.size(), val); + OCIO_CHECK_ASSERT(res.ptr == str.data() + 4); + OCIO_CHECK_EQUAL(val, 24.0f); + +#undef TEST_FROM_CHARS +} + +OCIO_ADD_TEST(NumberUtils, from_chars_float_failures) +{ +#define TEST_FROM_CHARS(text) \ + str = text; \ + res = OCIO::NumberUtils::from_chars(str.data(), str.data() + str.size(), val); \ + OCIO_CHECK_EQUAL(val, 7.5f); \ + OCIO_CHECK_ASSERT(res.ec == std::errc::invalid_argument) + + std::string str; + float val = 7.5f; + OCIO::NumberUtils::from_chars_result res; + + TEST_FROM_CHARS(""); + TEST_FROM_CHARS("ab"); + TEST_FROM_CHARS(" "); + TEST_FROM_CHARS("---"); + TEST_FROM_CHARS("e3"); + TEST_FROM_CHARS("_x"); + TEST_FROM_CHARS("+."); + +#undef TEST_FROM_CHARS +} From 842838140e7739d67997dce6f7b1269f726147e5 Mon Sep 17 00:00:00 2001 From: Aras Pranckevicius Date: Wed, 2 Oct 2024 11:06:10 +0300 Subject: [PATCH 4/7] Fix from_chars in C++17 code path to understand hex prefix (0x) and skip optional whitespace To match the pre-C++17 behavior that was there before Signed-off-by: Aras Pranckevicius --- src/utils/NumberUtils.h | 57 +++++++++++++++---- .../xmlutils/XMLReaderUtils_tests.cpp | 28 ++++----- 2 files changed, 62 insertions(+), 23 deletions(-) diff --git a/src/utils/NumberUtils.h b/src/utils/NumberUtils.h index 05231c4d2a..e4b5426592 100644 --- a/src/utils/NumberUtils.h +++ b/src/utils/NumberUtils.h @@ -11,7 +11,7 @@ // // Except on Apple platforms, where (as of Xcode 15 / Apple Clang 15) // these are not implemented for float/double types. -#if __cpp_lib_to_chars >= 201611L && !defined(__APPLE__) +#if __cplusplus >= 201703L && !defined(__APPLE__) #define USE_CHARCONV_FROM_CHARS #include #endif @@ -66,6 +66,37 @@ struct from_chars_result static const Locale loc; +#ifdef USE_CHARCONV_FROM_CHARS +really_inline bool from_chars_is_space(char c) noexcept +{ + return c == ' ' || c == '\n' || c == '\t' || c == '\r' || c == '\v' || c == '\f'; +} + +// skip prefix whitespace and "+" +really_inline const char* from_chars_skip_prefix(const char* first, const char* last) noexcept +{ + while (first < last && from_chars_is_space(first[0])) + { + ++first; + } + if (first < last && first[0] == '+') + { + ++first; + } + return first; +} + +really_inline bool from_chars_hex_prefix(const char*& first, const char* last) noexcept +{ + if (first + 2 < last && first[0] == '0' && (first[1] == 'x' || first[1] == 'X')) + { + first += 2; + return true; + } + return false; +} +#endif + really_inline from_chars_result from_chars(const char *first, const char *last, double &value) noexcept { if (!first || !last || first == last) @@ -74,11 +105,13 @@ really_inline from_chars_result from_chars(const char *first, const char *last, } #ifdef USE_CHARCONV_FROM_CHARS - if (first < last && first[0] == '+') + first = from_chars_skip_prefix(first, last); + std::chars_format fmt = std::chars_format::general; + if (from_chars_hex_prefix(first, last)) { - ++first; + fmt = std::chars_format::hex; } - std::from_chars_result res = std::from_chars(first, last, value); + std::from_chars_result res = std::from_chars(first, last, value, fmt); return from_chars_result{ res.ptr, res.ec }; #else @@ -120,11 +153,13 @@ really_inline from_chars_result from_chars(const char *first, const char *last, } #ifdef USE_CHARCONV_FROM_CHARS - if (first < last && first[0] == '+') + first = from_chars_skip_prefix(first, last); + std::chars_format fmt = std::chars_format::general; + if (from_chars_hex_prefix(first, last)) { - ++first; + fmt = std::chars_format::hex; } - std::from_chars_result res = std::from_chars(first, last, value); + std::from_chars_result res = std::from_chars(first, last, value, fmt); return from_chars_result{ res.ptr, res.ec }; #else @@ -174,11 +209,13 @@ really_inline from_chars_result from_chars(const char *first, const char *last, } #ifdef USE_CHARCONV_FROM_CHARS - if (first < last && first[0] == '+') + first = from_chars_skip_prefix(first, last); + int base = 10; + if (from_chars_hex_prefix(first, last)) { - ++first; + base = 16; } - std::from_chars_result res = std::from_chars(first, last, value); + std::from_chars_result res = std::from_chars(first, last, value, base); return from_chars_result{ res.ptr, res.ec }; #else diff --git a/tests/cpu/fileformats/xmlutils/XMLReaderUtils_tests.cpp b/tests/cpu/fileformats/xmlutils/XMLReaderUtils_tests.cpp index ad33e496ef..44aa7d59fb 100644 --- a/tests/cpu/fileformats/xmlutils/XMLReaderUtils_tests.cpp +++ b/tests/cpu/fileformats/xmlutils/XMLReaderUtils_tests.cpp @@ -46,12 +46,21 @@ OCIO_ADD_TEST(XMLReaderHelper, string_to_float_failure) const char str2[] = "12345"; const size_t len2 = std::strlen(str2); // All characters are parsed and this is more than the required length. - // The string to double function strtod does not stop at a given length, - // but we detect that strtod did read too many characters. - OCIO_CHECK_THROW_WHAT(OCIO::ParseNumber(str2, 0, len2 - 2, value), - OCIO::Exception, - "followed by unexpected characters"); - + // The string to double function might not stop at a given length, + // but we detect if it did read too many characters. + try + { + OCIO::ParseNumber(str2, 0, len2 - 2, value); + // At this point value should be 123 if underlying implementation + // properly stops at given length. + OCIO_CHECK_EQUAL(value, 123.0f); + } + catch (OCIO::Exception const& ex) + { + // If underlying implementation scans past the end, exception should be thrown. + const std::string what(ex.what()); + OCIO_CHECK_ASSERT(what.find("followed by unexpected characters") != std::string::npos); + } const char str3[] = "123XX"; const size_t len3 = std::strlen(str3); @@ -385,13 +394,6 @@ OCIO_ADD_TEST(XMLReaderHelper, parse_number) OCIO_CHECK_EQUAL(data, 1.0f); } - { - std::string buffer(" 123 "); - OCIO_CHECK_THROW_WHAT(OCIO::ParseNumber(buffer.c_str(), - 0, 3, data), - OCIO::Exception, - "followed by unexpected characters"); - } { std::string buffer("XY"); OCIO_CHECK_THROW_WHAT(OCIO::ParseNumber(buffer.c_str(), From 3353ac420a6d9227855d89ec17e39f2386198aac Mon Sep 17 00:00:00 2001 From: Aras Pranckevicius Date: Wed, 2 Oct 2024 12:53:38 +0300 Subject: [PATCH 5/7] Fix detection of float from_chars availability Signed-off-by: Aras Pranckevicius --- src/utils/NumberUtils.h | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/utils/NumberUtils.h b/src/utils/NumberUtils.h index e4b5426592..e6b273f449 100644 --- a/src/utils/NumberUtils.h +++ b/src/utils/NumberUtils.h @@ -9,9 +9,13 @@ // That has advantage of not dealing with locale / errno, // which might involve locks or thread local storage accesses. // -// Except on Apple platforms, where (as of Xcode 15 / Apple Clang 15) -// these are not implemented for float/double types. -#if __cplusplus >= 201703L && !defined(__APPLE__) +// Note that it is not enough to check __cplusplus version, +// since even if compiler reports C++17 it might not implement +// from_chars for floats. Checking __cpp_lib_to_chars works +// correctly and the check starts passing with gcc 11, clang 12, +// msvc 2019 (16.4). It correctly does not pass on apple clang 15, +// since it does not implement from_chars for floats. +#if __cpp_lib_to_chars >= 201611L #define USE_CHARCONV_FROM_CHARS #include #endif From 3f65bb3182ca80a696f02db3c9ec76043f7dfe9c Mon Sep 17 00:00:00 2001 From: Aras Pranckevicius Date: Wed, 2 Oct 2024 13:46:01 +0300 Subject: [PATCH 6/7] Tests: Fix missing include for gcc Signed-off-by: Aras Pranckevicius --- tests/utils/NumberUtils_tests.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/utils/NumberUtils_tests.cpp b/tests/utils/NumberUtils_tests.cpp index faaeba334f..eaaa23dcb0 100644 --- a/tests/utils/NumberUtils_tests.cpp +++ b/tests/utils/NumberUtils_tests.cpp @@ -5,6 +5,8 @@ #include "testutils/UnitTest.h" #include "utils/NumberUtils.h" +#include + namespace OCIO = OCIO_NAMESPACE; OCIO_ADD_TEST(NumberUtils, from_chars_float) From 53fbca90f83176fe2df02a4fe706390d843d29a6 Mon Sep 17 00:00:00 2001 From: Aras Pranckevicius Date: Wed, 2 Oct 2024 13:56:45 +0300 Subject: [PATCH 7/7] Tests: fix uninitialized variable warning-as-error on gcc Signed-off-by: Aras Pranckevicius --- tests/utils/NumberUtils_tests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/utils/NumberUtils_tests.cpp b/tests/utils/NumberUtils_tests.cpp index eaaa23dcb0..ba03cd9a7f 100644 --- a/tests/utils/NumberUtils_tests.cpp +++ b/tests/utils/NumberUtils_tests.cpp @@ -18,7 +18,7 @@ OCIO_ADD_TEST(NumberUtils, from_chars_float) OCIO_CHECK_ASSERT(res.ptr == str.data() + str.size()) std::string str; - float val; + float val = 0.0f; OCIO::NumberUtils::from_chars_result res; // regular numbers