diff --git a/share/cmake/utils/CompilerFlags.cmake b/share/cmake/utils/CompilerFlags.cmake index d18d7fe7cc..536b5eebd5 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..e6b273f449 100644 --- a/src/utils/NumberUtils.h +++ b/src/utils/NumberUtils.h @@ -4,6 +4,22 @@ #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. +// +// 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 + #if defined(_MSC_VER) #define really_inline __forceinline #else @@ -54,14 +70,56 @@ 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 { - errno = 0; if (!first || !last || first == last) { return {first, std::errc::invalid_argument}; } +#ifdef USE_CHARCONV_FROM_CHARS + first = from_chars_skip_prefix(first, last); + std::chars_format fmt = std::chars_format::general; + if (from_chars_hex_prefix(first, last)) + { + fmt = std::chars_format::hex; + } + std::from_chars_result res = std::from_chars(first, last, value, fmt); + return from_chars_result{ res.ptr, res.ec }; +#else + + errno = 0; char * endptr = nullptr; double @@ -88,16 +146,28 @@ 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 + first = from_chars_skip_prefix(first, last); + std::chars_format fmt = std::chars_format::general; + if (from_chars_hex_prefix(first, last)) + { + fmt = std::chars_format::hex; + } + std::from_chars_result res = std::from_chars(first, last, value, fmt); + return from_chars_result{ res.ptr, res.ec }; +#else + + errno = 0; char *endptr = nullptr; float @@ -132,16 +202,28 @@ 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 + first = from_chars_skip_prefix(first, last); + int base = 10; + if (from_chars_hex_prefix(first, last)) + { + base = 16; + } + std::from_chars_result res = std::from_chars(first, last, value, base); + return from_chars_result{ res.ptr, res.ec }; +#else + + errno = 0; char *endptr = nullptr; long int @@ -170,6 +252,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..7dbfaa1a4b 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(unsigned 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 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(), 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..ba03cd9a7f --- /dev/null +++ b/tests/utils/NumberUtils_tests.cpp @@ -0,0 +1,83 @@ +// SPDX-License-Identifier: BSD-3-Clause +// Copyright Contributors to the OpenColorIO Project. + + +#include "testutils/UnitTest.h" +#include "utils/NumberUtils.h" + +#include + +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 = 0.0f; + 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 +}