From 3b668cf57f655c85c735f6a4b894fb41a373f1c8 Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Tue, 10 Oct 2017 14:25:49 -0700 Subject: [PATCH 1/2] More principled handling of float<->string This refactor comes after a belated realization that our numeric conversion routines (principally from_string<>) are "locale-dependent" via their use of strtod, as well as atof() scattered through the code. In particular, this means that when software using this code is running with a locale in which the decimal mark is something other than '.' (e.g., many European locales that use ',' for decimal mark), these functions will incorrectly parse text floating point numbers that use the standard period as decimal mark. For example, if the locale is "fr_FR", atof("123.45") will return 123.0, rather than the correct 123.45. At the core of our fix is the introduction of Strutil::strtof/strtod which by default is "locale-independent" (i.e. always uses '.') and also has varieties that take a std::locale& explicitly. The implementation uses strtod_l/strtof_l (nonstandard, but available on Linux, OSX, BSD, and a differently-named but equivalent function on Windows), with a gross but simple fallback for any straggler platforms (MINGW?) that copies the string and replaces the '.' with the native locale's mark. This is pretty inexpensive: on Linux, the locale-independent version of our new Strutil::strtof has the same cost as the system locale-dependent atof/strtof, and the explicit locale version is only about 20% slower. On OSX, atof is remarkably fast, so our version that uses strtof_l is about 2x slower, but that seems acceptable given that it gives us all the control we want, and in comparison, another strategy involving istringstream was 10x slower. Unless this is shown to be an important bottleneck, I'm rejecting the alternative of embedding a full custom strtod implementation as being unnecessarily inviting maintenance costs and potential bugs. The approach we chose is minimally invasive and relies on system libraries for the heavy lifting. Additionally: * Added a new stof(), stoi(), stoul() to replace the clunkier template syntax of from_string<>. The stof(), like our new strtof(), has a locale-independent as well as an explicit-local versions. * A handful of places that still had old calls to std atof and strtod were changed to Strutil::stof. * Strutil::format and ustring::format (and our copy of tinyformat underneath) were touched up so that the versions of format() that return strings will use the "C" locale, versions added that return strings and use an explicitly-passed locale, and the versions that append their results to existing streams continue to operate as before by honoring the existing locale conventions of the streams they are using. * Several places where we assembled strings using std::ostringstream, I forced the stream to use classic "C" locale in cases where it was likely to have any floating-point data output. * For maketx and oiiotool, globally set the locale to classic. In these cases, we control the whole app, so this is safe to do. TIP going forward: For any I/O that must be persistent (files saved or read), or always be formatted identically regardless of the computer being used, always take care to use locale-independent (i.e. classic "C" locale) formatting functions and also making sure to initialize any std::ostringstream with stream.imbue(std::locale::classic()). For ephemeral I/O or UI elements that you want to display correctly internationalized for the user's country, then use the versions with explicit locale std::locale() and use the default initialization of output streams. As an aside, I do wish that C/C++ had adopted the convention of default using the classic (globally uniform) locale for all functions that don't take an explicit locale, and leave the explicit locale functions just for programs that are intentionally trying to do country-by-country localization. This is all just a huge mess, and I hate that our library can have subtle I/O bugs when used by someone in another country because of a particular global locale setting that we have no control over and can't modify simply without potentially breaking the surrounding app. --- .travis.yml | 1 + src/fits.imageio/fitsinput.cpp | 2 +- src/include/OpenImageIO/optparser.h | 5 +- src/include/OpenImageIO/strutil.h | 110 +++++++++++++++++-- src/include/OpenImageIO/tinyformat.h | 32 +++++- src/include/OpenImageIO/ustring.h | 20 ++++ src/iv/imageviewer.cpp | 9 +- src/libOpenImageIO/formatspec.cpp | 1 + src/libOpenImageIO/maketexture.cpp | 3 + src/libtexture/imagecache.cpp | 2 + src/libtexture/texturesys.cpp | 1 + src/libutil/argparse.cpp | 4 +- src/libutil/benchmark.cpp | 16 ++- src/libutil/strutil.cpp | 153 ++++++++++++++++++++++++++- src/libutil/strutil_test.cpp | 73 +++++++++++++ src/libutil/ustring.cpp | 1 + src/maketx/maketx.cpp | 4 + src/oiiotool/oiiotool.cpp | 8 +- src/rla.imageio/rlainput.cpp | 2 +- 19 files changed, 415 insertions(+), 32 deletions(-) diff --git a/.travis.yml b/.travis.yml index 43d27228ad..7996c868cf 100644 --- a/.travis.yml +++ b/.travis.yml @@ -32,6 +32,7 @@ addons: - libavformat-dev - libswscale-dev - libavutil-dev + - locales # - qtbase5-dev # FIXME: enable Qt5 on Linux # - bzip2 # - libtinyxml-dev diff --git a/src/fits.imageio/fitsinput.cpp b/src/fits.imageio/fitsinput.cpp index a8e1218b96..80f811b22e 100644 --- a/src/fits.imageio/fitsinput.cpp +++ b/src/fits.imageio/fitsinput.cpp @@ -348,7 +348,7 @@ FitsInput::add_to_spec (const std::string &keyname, const std::string &value) // converting string to float or integer bool isNumSign = (value[0] == '+' || value[0] == '-' || value[0] == '.'); if (isdigit (value[0]) || isNumSign) { - float val = atof (value.c_str ()); + float val = Strutil::stof (value); if (val == (int)val) m_spec.attribute (keyname, (int)val); else diff --git a/src/include/OpenImageIO/optparser.h b/src/include/OpenImageIO/optparser.h index dcf8ce0349..6c57ec786e 100644 --- a/src/include/OpenImageIO/optparser.h +++ b/src/include/OpenImageIO/optparser.h @@ -40,6 +40,7 @@ #define OPENIMAGEIO_OPTPARSER_H #include +#include OIIO_NAMESPACE_BEGIN @@ -67,9 +68,9 @@ optparse1 (C &system, const std::string &opt) char v = value.size() ? value[0] : ' '; if ((v >= '0' && v <= '9') || v == '+' || v == '-') { // numeric if (strchr (value.c_str(), '.')) // float - return system.attribute (name.c_str(), (float)atof(value.c_str())); + return system.attribute (name, Strutil::stof(value)); else // int - return system.attribute (name.c_str(), (int)atoi(value.c_str())); + return system.attribute (name, Strutil::stoi(value)); } // otherwise treat it as a string diff --git a/src/include/OpenImageIO/strutil.h b/src/include/OpenImageIO/strutil.h index 6d034d5067..5b677cee64 100644 --- a/src/include/OpenImageIO/strutil.h +++ b/src/include/OpenImageIO/strutil.h @@ -97,23 +97,41 @@ void OIIO_API sync_output (std::ostream &file, string_view str); /// /// Uses the tinyformat library underneath, so it's fully type-safe, and /// works with any types that understand stream output via '<<'. +/// The formatting of the string will always use the classic "C" locale +/// conventions (in particular, '.' as decimal separator for float values). template inline std::string format (string_view fmt, const Args&... args) { return tinyformat::format (fmt.c_str(), args...); } +/// A version of Strutil::format() that uses an explicit locale. +template +inline std::string format (const std::locale& loc, string_view fmt, + const Args&... args) +{ + return tinyformat::format (loc, fmt.c_str(), args...); +} + /// Output formatted string to stdout, type-safe, and threads can't clobber -/// one another. +/// one another. This will force the classic "C" locale. template inline void printf (string_view fmt, const Args&... args) { sync_output (stdout, format(fmt, args...)); } +/// Output to stdout using an explicit locale. If you want it to exactly +/// match the locale of std::cout, std::cout.getloc() as the locale. +template +inline void printf (const std::locale& loc, string_view fmt, const Args&... args) +{ + sync_output (stdout, format(loc, fmt, args...)); +} + /// Output formatted string to an open FILE*, type-safe, and threads can't -/// clobber one another. +/// clobber one another. This will force classic "C" locale conventions. template inline void fprintf (FILE *file, string_view fmt, const Args&... args) { @@ -121,11 +139,12 @@ inline void fprintf (FILE *file, string_view fmt, const Args&... args) } /// Output formatted string to an open ostream, type-safe, and threads can't -/// clobber one another. +/// clobber one another. This will honor the existing locale conventions of +/// the ostream. template inline void fprintf (std::ostream &file, string_view fmt, const Args&... args) { - sync_output (file, format(fmt, args...)); + sync_output (file, format(file.getloc(), fmt, args...)); } @@ -254,6 +273,23 @@ std::string OIIO_API repeat (string_view str, int n); std::string OIIO_API replace (string_view str, string_view pattern, string_view replacement, bool global=false); + +/// strtod/strtof equivalents that are "locale-independent", always using +/// '.' as the decimal separator. This should be preferred for I/O and other +/// situations where you want the same standard formatting regardless of +/// locale. +float OIIO_API strtof (const char *nptr, char **endptr = nullptr); +double OIIO_API strtod (const char *nptr, char **endptr = nullptr); + +/// strtod/strtof equivalents that take an explicit locale. This may be +/// useful for UI or I/O that you specifically want to use the conventions +/// of a localized country. +double OIIO_API strtod (const char *nptr, char **endptr, const std::locale& loc); +float OIIO_API strtof (const char *nptr, char **endptr, const std::locale& loc); +#define OIIO_STRUTIL_HAS_STRTOF 1 /* be able to test this */ + + + // Helper template to test if a string is a generic type template inline bool string_is (string_view /*s*/) { @@ -267,33 +303,85 @@ template <> inline bool string_is (string_view s) { strtol (s.data(), &endptr, 10); return (s.data() + s.size() == endptr); } -// Special case for float +// Special case for float. Note that by using Strutil::strtof, this always +// treats '.' as the decimal character. template <> inline bool string_is (string_view s) { if (s.empty()) return false; char *endptr = 0; - strtod (s.data(), &endptr); + Strutil::strtof (s.data(), &endptr); return (s.data() + s.size() == endptr); } -// Helper template to convert from generic type to string +// stoi() returns the int conversion of text from several string types. +// No exceptions or errors -- parsing errors just return 0. +inline int stoi (const char* s) { + return s && s[0] ? strtol (s, nullptr, 10) : 0; +} +inline int stoi (const std::string& s) { return Strutil::stoi(s.c_str()); } +inline int stoi (string_view s) { return Strutil::stoi (std::string(s)); } + + + +// stoul() returns the unsigned int conversion of text from several string +// types. No exceptions or errors -- parsing errors just return 0. +inline unsigned int stoul (const char* s) { + return s && s[0] ? strtoul (s, nullptr, 10) : 0; +} +inline unsigned int stoul (const std::string& s) { return Strutil::stoul(s.c_str()); } +inline unsigned int stoul (string_view s) { return Strutil::stoul (std::string(s)); } + + + +/// stof() returns the float conversion of text from several string types. +/// No exceptions or errors -- parsing errors just return 0.0. These always +/// use '.' for the decimal mark (versus atof and std::strtof, which are +/// locale-dependent). +inline float stof (const std::string& s) { + return s.size() ? Strutil::strtof (s.c_str(), nullptr) : 0.0f; +} +inline float stof (const char* s) { + return Strutil::strtof (s, nullptr); +} +inline float stof (string_view s) { + return Strutil::strtof (std::string(s).c_str(), nullptr); +} + +// stof() version that takes an explicit locale (for example, if you pass a +// default-constructed std::locale, it will use the current native locale's +// decimal conventions). +inline float stof (const std::string& s, const std::locale& loc) { + return s.size() ? Strutil::strtof (s.c_str(), nullptr, loc) : 0.0f; +} +inline float stof (const char* s, const std::locale& loc) { + return Strutil::strtof (s, nullptr, loc); +} +inline float stof (string_view s, const std::locale& loc) { + return Strutil::strtof (std::string(s).c_str(), nullptr, loc); +} + + + +// Helper template to convert from generic type to string (using default +// locale). template inline T from_string (string_view s) { return T(s); // Generic: assume there is an explicit converter } // Special case for int template<> inline int from_string (string_view s) { - return s.size() ? strtol (s.c_str(), NULL, 10) : 0; + return s.size() ? strtol (s.c_str(), nullptr, 10) : 0; } // Special case for uint template<> inline unsigned int from_string (string_view s) { - return s.size() ? strtoul (s.c_str(), NULL, 10) : (unsigned int)0; + return s.size() ? strtoul (s.c_str(), nullptr, 10) : (unsigned int)0; } -// Special case for float +// Special case for float -- note that by using Strutil::strtof, this +// always treats '.' as the decimal mark. template<> inline float from_string (string_view s) { - return s.size() ? (float)strtod (s.c_str(), NULL) : 0.0f; + return s.size() ? Strutil::strtof (s.c_str(), nullptr) : 0.0f; } diff --git a/src/include/OpenImageIO/tinyformat.h b/src/include/OpenImageIO/tinyformat.h index 0449875885..f6cc94680b 100644 --- a/src/include/OpenImageIO/tinyformat.h +++ b/src/include/OpenImageIO/tinyformat.h @@ -266,6 +266,7 @@ template inline void formatTruncated(std::ostream& out, const T& value, int ntrunc) { std::ostringstream tmp; + tmp.imbue (out.getloc()); tmp << value; std::string result = tmp.str(); out.write(result.c_str(), (std::min)(ntrunc, static_cast(result.size()))); @@ -800,6 +801,7 @@ inline void formatImpl(std::ostream& out, const char* fmt, // it crudely by formatting into a temporary string stream and // munging the resulting string. std::ostringstream tmpStream; + tmpStream.imbue (out.getloc()); tmpStream.copyfmt(out); tmpStream.setf(std::ios::showpos); arg.format(tmpStream, fmt, fmtEnd, ntrunc); @@ -931,6 +933,7 @@ TINYFORMAT_FOREACH_ARGNUM(TINYFORMAT_MAKE_MAKEFORMATLIST) #endif /// Format list of arguments to the stream according to the given format string. +/// This honors the stream's existing locale conventions. /// /// The name vformat() is chosen for the semantic similarity to vprintf(): the /// list of format arguments is held in a single function argument. @@ -943,6 +946,7 @@ inline void vformat(std::ostream& out, const char* fmt, FormatListRef list) #ifdef TINYFORMAT_USE_VARIADIC_TEMPLATES /// Format list of arguments to the stream according to given format string. +/// This honors the stream's existing locale conventions. template void format(std::ostream& out, const char* fmt, const Args&... args) { @@ -950,16 +954,32 @@ void format(std::ostream& out, const char* fmt, const Args&... args) } /// Format list of arguments according to the given format string and return -/// the result as a string. +/// the result as a string, using classic "C" locale conventions (e.g., +/// using '.' as decimal mark). template std::string format(const char* fmt, const Args&... args) { std::ostringstream oss; + oss.imbue (std::locale::classic()); // force "C" locale with '.' decimal + format(oss, fmt, args...); + return oss.str(); +} + +/// Format list of arguments according to the given format string and return +/// the result as a string, using an explicit locale. Passing loc as a +/// default-constructed std::locale will result in adhering to the current +/// "native" locale set by the OS. +template +std::string format(const std::locale& loc, const char* fmt, const Args&... args) +{ + std::ostringstream oss; + oss.imbue (loc); format(oss, fmt, args...); return oss.str(); } /// Format list of arguments to std::cout, according to the given format string +/// This honors std::out's existing locale conventions. template void printf(const char* fmt, const Args&... args) { @@ -1011,6 +1031,16 @@ template \ std::string format(const char* fmt, TINYFORMAT_VARARGS(n)) \ { \ std::ostringstream oss; \ + oss.imbue (std::locale::classic()); \ + format(oss, fmt, TINYFORMAT_PASSARGS(n)); \ + return oss.str(); \ +} \ + \ +template \ +std::string format(const std::locale& loc, const char* fmt, TINYFORMAT_VARARGS(n)) \ +{ \ + std::ostringstream oss; \ + oss.imbue (loc); \ format(oss, fmt, TINYFORMAT_PASSARGS(n)); \ return oss.str(); \ } \ diff --git a/src/include/OpenImageIO/ustring.h b/src/include/OpenImageIO/ustring.h index 59b0dd26a8..734795e374 100644 --- a/src/include/OpenImageIO/ustring.h +++ b/src/include/OpenImageIO/ustring.h @@ -626,12 +626,22 @@ class OIIO_API ustring { /// something like: /// ustring s = ustring::format ("blah %d %g", (int)foo, (float)bar); /// The argument list is fully typesafe. + /// The formatting of the string will always use the classic "C" locale + /// conventions (in particular, '.' as decimal separator for float values). template static ustring format (string_view fmt, const Args&... args) { return ustring (Strutil::format (fmt, args...)); } + /// ustring formatting with an explicit locale. + template + static ustring format (const std::locale& loc, string_view fmt, + const Args&... args) + { + return ustring (Strutil::format (loc, fmt, args...)); + } + /// Generic stream output of a ustring. /// friend std::ostream & operator<< (std::ostream &out, const ustring &str) { @@ -751,6 +761,16 @@ inline bool iequals (const std::string &a, ustring b) { } + +// add ustring variants of stoi and stof from OpenImageIO/strutil.h +namespace Strutil { +inline int stoi (ustring s) { return Strutil::stoi (s.c_str()); } +inline int stof (ustring s) { return Strutil::strtof (s.c_str()); } +inline int stof (ustring s, const std::locale& loc) { + return Strutil::strtof (s.c_str(), nullptr, loc); +} +} // end namespace Strutil + OIIO_NAMESPACE_END #endif // OPENIMAGEIO_USTRING_H diff --git a/src/iv/imageviewer.cpp b/src/iv/imageviewer.cpp index 91c898eae4..f0074d8c59 100644 --- a/src/iv/imageviewer.cpp +++ b/src/iv/imageviewer.cpp @@ -117,12 +117,9 @@ ImageViewer::ImageViewer () { readSettings (false); - const char *gamenv = getenv ("GAMMA"); - if (gamenv) { - float g = atof (gamenv); - if (g >= 0.1 && g <= 5) - m_default_gamma = g; - } + float gam = Strutil::stof (Sysutil::getenv ("GAMMA")); + if (gam >= 0.1 && gam <= 5) + m_default_gamma = gam; // FIXME -- would be nice to have a more nuanced approach to display // color space, in particular knowing whether the display is sRGB. // Also, some time in the future we may want a real 3D LUT for diff --git a/src/libOpenImageIO/formatspec.cpp b/src/libOpenImageIO/formatspec.cpp index 4afb42c4bc..1d0a9debcc 100644 --- a/src/libOpenImageIO/formatspec.cpp +++ b/src/libOpenImageIO/formatspec.cpp @@ -936,6 +936,7 @@ spec_to_xml (const ImageSpec &spec, ImageSpec::SerialVerbose verbose) } std::ostringstream result; + result.imbue (std::locale::classic()); // force "C" locale with '.' decimal doc.print (result, ""); return result.str(); } diff --git a/src/libOpenImageIO/maketexture.cpp b/src/libOpenImageIO/maketexture.cpp index b566291870..3e87af68a7 100644 --- a/src/libOpenImageIO/maketexture.cpp +++ b/src/libOpenImageIO/maketexture.cpp @@ -1386,6 +1386,7 @@ make_texture_impl (ImageBufAlgo::MakeTextureMode mode, // (such as filtering information) needs to be manually added into the // hash. std::ostringstream addlHashData; + addlHashData.imbue (std::locale::classic()); // Force "C" locale with '.' decimal addlHashData << filtername << " "; float sharpen = configspec.get_float_attribute ("maketx:sharpen", 0.0f); if (sharpen != 0.0f) { @@ -1417,6 +1418,7 @@ make_texture_impl (ImageBufAlgo::MakeTextureMode mode, if (isConstantColor) { std::ostringstream os; // Emulate a JSON array + os.imbue (std::locale::classic()); // Force "C" locale with '.' decimal for (int i = 0; i < dstspec.nchannels; ++i) { if (i!=0) os << ","; os << (i<(int)constantColor.size() ? constantColor[i] : 0.0f); @@ -1436,6 +1438,7 @@ make_texture_impl (ImageBufAlgo::MakeTextureMode mode, if (compute_average_color) { std::ostringstream os; // Emulate a JSON array + os.imbue (std::locale::classic()); // Force "C" locale with '.' decimal for (int i = 0; i < dstspec.nchannels; ++i) { if (i!=0) os << ","; os << (i<(int)pixel_stats.avg.size() ? pixel_stats.avg[i] : 0.0f); diff --git a/src/libtexture/imagecache.cpp b/src/libtexture/imagecache.cpp index 17330f5771..60fec4fd8f 100644 --- a/src/libtexture/imagecache.cpp +++ b/src/libtexture/imagecache.cpp @@ -1610,6 +1610,7 @@ ImageCacheImpl::onefile_stat_line (const ImageCacheFileRef &file, { // FIXME -- make meaningful stat printouts for multi-image textures std::ostringstream out; + out.imbue (std::locale::classic()); // Force "C" locale with '.' decimal const ImageSpec &spec (file->spec(0,0)); const char *formatcode = "u8"; switch (spec.format.basetype) { @@ -1737,6 +1738,7 @@ ImageCacheImpl::getstats (int level) const } std::ostringstream out; + out.imbue (std::locale::classic()); // Force "C" locale with '.' decimal if (level > 0) { out << "OpenImageIO ImageCache statistics ("; { diff --git a/src/libtexture/texturesys.cpp b/src/libtexture/texturesys.cpp index c3275f375f..17c4936f19 100644 --- a/src/libtexture/texturesys.cpp +++ b/src/libtexture/texturesys.cpp @@ -379,6 +379,7 @@ TextureSystemImpl::getstats (int level, bool icstats) const m_imagecache->mergestats (stats); std::ostringstream out; + out.imbue (std::locale::classic()); // Force "C" locale with '.' decimal bool anytexture = (stats.texture_queries + stats.texture3d_queries + stats.shadow_queries + stats.environment_queries); if (level > 0 && anytexture) { diff --git a/src/libutil/argparse.cpp b/src/libutil/argparse.cpp index b128b728b6..c873bf8132 100644 --- a/src/libutil/argparse.cpp +++ b/src/libutil/argparse.cpp @@ -252,11 +252,11 @@ ArgOption::set_parameter (int i, const char *argv) case 'f': case 'g': - *(float *)m_param[i] = (float)atof(argv); + *(float *)m_param[i] = Strutil::stof(argv); break; case 'F': - *(double *)m_param[i] = atof(argv); + *(double *)m_param[i] = Strutil::strtod(argv); break; case 's': diff --git a/src/libutil/benchmark.cpp b/src/libutil/benchmark.cpp index f4259457fa..6099a0860c 100644 --- a/src/libutil/benchmark.cpp +++ b/src/libutil/benchmark.cpp @@ -144,6 +144,12 @@ std::ostream& operator<< (std::ostream& out, const Benchmarker &bench) } const char* unitname = unitnames[unit]; double scale = unitscales[unit]; + char rateunit = 'M'; + double ratescale = 1.0e6; + if (bench.avg() >= 1.0e-6) { + rateunit = 'k'; + ratescale = 1.0e3; + } avg *= scale; stddev *= scale; @@ -163,12 +169,12 @@ std::ostream& operator<< (std::ostream& out, const Benchmarker &bench) return out; } if (bench.work() == 1) - out << Strutil::format ("%6.1f M/s", - (1.0f/1.0e6)/bench.avg()); + out << Strutil::format ("%6.1f %c/s", + (1.0f/ratescale)/bench.avg(), rateunit); else - out << Strutil::format ("%6.1f Mvals/s, %.1f Mcalls/s", - (bench.work()/1.0e6)/bench.avg(), - (1.0f/1.0e6)/bench.avg()); + out << Strutil::format ("%6.1f %cvals/s, %.1f %ccalls/s", + (bench.work()/ratescale)/bench.avg(), rateunit, + (1.0f/ratescale)/bench.avg(), rateunit); if (bench.verbose() >= 2) out << Strutil::format (" (%dx%d, rng=%.1f%%, med=%.1f)", bench.trials(), bench.iterations(), unitname, diff --git a/src/libutil/strutil.cpp b/src/libutil/strutil.cpp index 80de761f65..8aef69b296 100644 --- a/src/libutil/strutil.cpp +++ b/src/libutil/strutil.cpp @@ -37,6 +37,10 @@ #include #include #include +#include +#if defined(__APPLE__) || defined(__FreeBSD__) +#include +#endif #include @@ -716,7 +720,7 @@ Strutil::parse_float (string_view &str, float &val, bool eat) if (! p.size()) return false; const char *end = p.begin(); - float v = (float) strtod (p.begin(), (char**)&end); + float v = Strutil::strtof (p.begin(), (char**)&end); if (end == p.begin()) return false; // no integer found if (eat) { @@ -1032,4 +1036,151 @@ Strutil::base64_encode (string_view str) } + +double +Strutil::strtod (const char *nptr, char **endptr, const std::locale& loc) +{ + // Get decimal point in requested locale + char pointchar = std::use_facet>(loc).decimal_point(); + + // If the requested locale uses '.' as decimal point, equivalent to C + // locale, and this OS has strtod_l, use it. +#if defined (__linux__) || defined(__APPLE__) || defined(__FreeBSD__) || defined(__FreeBSD_kernel__) + if (pointchar == '.') { + // static initialization inside function is thread-safe by C++11 rules! + static locale_t c_loc = newlocale(LC_ALL_MASK, "C", nullptr); + return strtod_l (nptr, endptr, c_loc); + } +#elif defined (_WIN32) + // Windows has _strtod_l + if (pointchar == '.') { + static _locale_t c_loc = _create_locale(LC_ALL, "C"); + return _strtod_l (nptr, endptr, c_loc); + } +#endif + + std::locale native; // default ctr gets current global locale + char nativepoint = std::use_facet>(native).decimal_point(); + + // If the requested and native locales use the same decimal character, + // just directly use strtod. + if (pointchar == nativepoint) + return ::strtod (nptr, endptr); + + // Complex case -- CHEAT by making a copy of the string and replacing + // the decimal, then use system strtod! + std::string s (nptr); + const char* pos = strchr (nptr, pointchar); + if (pos) { + s[pos-nptr] = nativepoint; + auto d = ::strtod (s.c_str(), endptr); + if (endptr) + *endptr = (char *)nptr + (*endptr - s.c_str()); + return d; + } + // No decimal point at all -- use regular strtod + return ::strtod (s.c_str(), endptr); +} + + + +float +Strutil::strtof (const char *nptr, char **endptr, const std::locale& loc) +{ + // Get decimal point in requested locale + char pointchar = std::use_facet>(loc).decimal_point(); + + // If the requested locale uses '.' as decimal point, equivalent to C + // locale, and this OS has strtod_l, use it. +#if defined (__linux__) || defined(__APPLE__) || defined(__FreeBSD__) || defined(__FreeBSD_kernel__) + if (pointchar == '.') { + // static initialization inside function is thread-safe by C++11 rules! + static locale_t c_loc = newlocale(LC_ALL_MASK, "C", nullptr); + return strtof_l (nptr, endptr, c_loc); + } +#elif defined (_WIN32) + // Windows has _strtod_l + if (pointchar == '.') { + static _locale_t c_loc = _create_locale(LC_ALL, "C"); + return (float) _strtod_l (nptr, endptr, c_loc); + } +#endif + + std::locale native; // default ctr gets current global locale + char nativepoint = std::use_facet>(native).decimal_point(); + + // If the requested and native locales use the same decimal character, + // just directly use strtof. + if (pointchar == nativepoint) + return ::strtof (nptr, endptr); + + // Complex case -- CHEAT by making a copy of the string and replacing + // the decimal, then use system strtod! + std::string s (nptr); + const char* pos = strchr (nptr, pointchar); + if (pos) { + s[pos-nptr] = nativepoint; + auto d = ::strtof (s.c_str(), endptr); + if (endptr) + *endptr = (char *)nptr + (*endptr - s.c_str()); + return d; + } + // No decimal point at all -- use regular strtod + return ::strtof (s.c_str(), endptr); +} + + +float +Strutil::strtof (const char *nptr, char **endptr) +{ + // Can use strtof_l on platforms that support it +#if defined (__linux__) || defined(__APPLE__) || defined(__FreeBSD__) || defined(__FreeBSD_kernel__) + // static initialization inside function is thread-safe by C++11 rules! + static locale_t c_loc = newlocale(LC_ALL_MASK, "C", nullptr); + return strtof_l (nptr, endptr, c_loc); +#elif defined (_WIN32) + // Windows has _strtod_l + static _locale_t c_loc = _create_locale(LC_ALL, "C"); + return (float) _strtod_l (nptr, endptr, c_loc); +#else + // On platforms without strtof_l, use the classic locale explicitly and + // call the locale-based version. + return strtof (nptr, endptr, std::locale::classic()); +#endif +} + + +double +Strutil::strtod (const char *nptr, char **endptr) +{ + // Can use strtod_l on platforms that support it +#if defined (__linux__) || defined(__APPLE__) || defined(__FreeBSD__) || defined(__FreeBSD_kernel__) + // static initialization inside function is thread-safe by C++11 rules! + static locale_t c_loc = newlocale(LC_ALL_MASK, "C", nullptr); + return strtod_l (nptr, endptr, c_loc); +#elif defined (_WIN32) + // Windows has _strtod_l + static _locale_t c_loc = _create_locale(LC_ALL, "C"); + return _strtod_l (nptr, endptr, c_loc); +#else + // On platforms without strtod_l, use the classic locale explicitly and + // call the locale-based version. + return strtod (nptr, endptr, std::locale::classic()); +#endif +} + +// Notes: +// +// FreeBSD's implementation of strtod: +// https://svnweb.freebsd.org/base/stable/10/contrib/gdtoa/strtod.c?view=markup +// Python's implementation of strtod: (BSD license) +// https://hg.python.org/cpython/file/default/Python/pystrtod.c +// Julia's implementation (combo of strtod_l and Python's impl): +// https://github.com/JuliaLang/julia/blob/master/src/support/strtod.c +// MSDN documentation on Windows _strtod_l and friends: +// https://msdn.microsoft.com/en-us/library/kxsfc1ab.aspx (_strtod_l) +// https://msdn.microsoft.com/en-us/library/4zx9aht2.aspx (_create_locale) +// cppreference on locale: +// http://en.cppreference.com/w/cpp/locale/locale + OIIO_NAMESPACE_END diff --git a/src/libutil/strutil_test.cpp b/src/libutil/strutil_test.cpp index c583432b5e..380fc69c61 100644 --- a/src/libutil/strutil_test.cpp +++ b/src/libutil/strutil_test.cpp @@ -30,6 +30,7 @@ #include +#include #include #include @@ -369,6 +370,28 @@ void test_conversion () OIIO_CHECK_EQUAL (Strutil::string_is(" 142"), true); OIIO_CHECK_EQUAL (Strutil::string_is("x142"), false); + OIIO_CHECK_EQUAL (Strutil::stoi("hi"), 0); + OIIO_CHECK_EQUAL (Strutil::stoi("123"), 123); + OIIO_CHECK_EQUAL (Strutil::stoi("-123"), -123); + OIIO_CHECK_EQUAL (Strutil::stoi(" 123 "), 123); + OIIO_CHECK_EQUAL (Strutil::stoi("123.45"), 123); + + OIIO_CHECK_EQUAL (Strutil::stoul("hi"), unsigned(0)); + OIIO_CHECK_EQUAL (Strutil::stoul("123"), unsigned(123)); + OIIO_CHECK_EQUAL (Strutil::stoul("-123"), unsigned(-123)); + OIIO_CHECK_EQUAL (Strutil::stoul(" 123 "), unsigned(123)); + OIIO_CHECK_EQUAL (Strutil::stoul("123.45"), unsigned(123)); + + OIIO_CHECK_EQUAL (Strutil::stof("hi"), 0.0f); + OIIO_CHECK_EQUAL (Strutil::stof("123"), 123.0f); + OIIO_CHECK_EQUAL (Strutil::stof("-123"), -123.0f); + OIIO_CHECK_EQUAL (Strutil::stof("123.45"), 123.45f); + OIIO_CHECK_EQUAL (Strutil::stof(" 123.45 "), 123.45f); + OIIO_CHECK_EQUAL (Strutil::stof("123.45+12"), 123.45f); + OIIO_CHECK_EQUAL (Strutil::stof("1.2345e+2"), 123.45f); + // stress case! + OIIO_CHECK_EQUAL (Strutil::stof("100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001E-200"), 1.0f); + OIIO_CHECK_EQUAL (Strutil::from_string("hi"), 0); OIIO_CHECK_EQUAL (Strutil::from_string("123"), 123); OIIO_CHECK_EQUAL (Strutil::from_string("-123"), -123); @@ -616,6 +639,55 @@ void test_parse () +// For comparision, use string stream read with locale +inline float stof_stream (const std::string& s, const std::locale& loc = std::locale::classic()) { + // WRONG but fast: return s.size() ? strtod (s.c_str(), nullptr, 10) : 0.0f; + float result; + std::istringstream stream(s); + stream.imbue(loc); + stream >> result; + return result; +} + + + +void +test_locale () +{ + std::cout << "Testing float conversion + locale\n"; + std::locale oldloc = std::locale::global(std::locale::classic()); // save original locale + Benchmarker bench; + bench.indent (2); + bench.units (Benchmarker::Unit::ns); + const char* numcstr = "123.45"; + std::string numstring (numcstr); + bench ("get default locale", [](){ std::locale loc; DoNotOptimize (loc); }); + bench ("ref classic locale", [](){ DoNotOptimize (std::locale::classic()); }); + bench ("copy classic locale", [](){ std::locale loc = std::locale::classic(); DoNotOptimize(loc); }); + bench ("get by-name locale", [](){ std::locale loc = std::locale("fr_FR.UTF-8"); DoNotOptimize(loc);}); + bench ("std atof", [&](){ DoNotOptimize(atof(numcstr));}); + bench ("std strtod", [&](){ DoNotOptimize(::strtod(numcstr, nullptr));}); + bench ("Strutil::from_string", [&](){ DoNotOptimize(Strutil::from_string(numstring));}); + bench ("stof_stream(string) - safe with locale", [&](){ return DoNotOptimize(stof_stream(numstring)); }); + bench ("Strutil::stof(string) - locale-independent", [&](){ return DoNotOptimize(Strutil::stof(numstring)); }); + bench ("Strutil::stof(string) - explicit classic locale", [&](){ return DoNotOptimize(Strutil::stof(numstring, std::locale::classic())); }); + bench ("Strutil::stof(string) - explicit native locale", [&](){ return DoNotOptimize(Strutil::stof(numstring, std::locale(""))); }); + bench ("locale switch (to classic)", [&](){ std::locale::global (std::locale::classic()); }); + + std::locale::global (std::locale("fr_FR.UTF-8")); + bench ("stof(string) - safe with weird locale", [&](){ return DoNotOptimize(Strutil::stof(numstring)); }); + std::cout << "safe float convert (C locale) " << numcstr << " = " << Strutil::stof(numcstr) << "\n"; + OIIO_CHECK_EQUAL_APPROX (Strutil::stof(numcstr), 123.45f); + std::cout << "unsafe float convert (default locale) " << numcstr << " = " << atof(numcstr) << "\n"; + OIIO_CHECK_EQUAL_APPROX (atof(numcstr), 123.0f); + // Verify that Strutil::format does the right thing, even when in a + // comma-based locale. + OIIO_CHECK_EQUAL (Strutil::format ("%g", 123.45f), "123.45"); + OIIO_CHECK_EQUAL (Strutil::format (std::locale(), "%g", 123.45f), "123,45"); + std::locale::global (oldloc); // restore +} + + void test_float_formatting () @@ -668,6 +740,7 @@ main (int argc, char *argv[]) test_safe_strcpy (); test_string_view (); test_parse (); + test_locale (); // test_float_formatting (); return unit_test_failures; diff --git a/src/libutil/ustring.cpp b/src/libutil/ustring.cpp index 90663355ce..31ed368481 100644 --- a/src/libutil/ustring.cpp +++ b/src/libutil/ustring.cpp @@ -406,6 +406,7 @@ ustring::getstats (bool verbose) { UstringTable &table (ustring_table()); std::ostringstream out; + out.imbue (std::locale::classic()); // Force "C" locale with '.' decimal size_t n_l = table.get_num_lookups(); size_t n_e = table.get_num_entries(); size_t mem = table.get_memory_usage(); diff --git a/src/maketx/maketx.cpp b/src/maketx/maketx.cpp index 9b740d64b7..ee59b34360 100644 --- a/src/maketx/maketx.cpp +++ b/src/maketx/maketx.cpp @@ -446,6 +446,10 @@ main (int argc, char *argv[]) { Timer alltimer; + // Globally force classic "C" locale, and turn off all formatting + // internationalization, for the entire maketx application. + std::locale::global (std::locale::classic()); + ImageSpec configspec; Filesystem::convert_native_arguments (argc, (const char **)argv); getargs (argc, argv, configspec); diff --git a/src/oiiotool/oiiotool.cpp b/src/oiiotool/oiiotool.cpp index c79e409208..adad15ec67 100644 --- a/src/oiiotool/oiiotool.cpp +++ b/src/oiiotool/oiiotool.cpp @@ -1341,7 +1341,7 @@ set_input_attribute (int argc, const char *argv[]) // Does it seem to be a float, or did the caller explicitly request // that it be set as a float? p = NULL; - float f = (float)strtod (value.c_str(), &p); + float f = Strutil::strtof (value.c_str(), &p); while (*p && isspace(*p)) ++p; if ((! *p && type == TypeDesc::UNKNOWN) || type == TypeDesc::FLOAT) { @@ -1495,7 +1495,7 @@ OiioTool::set_attribute (ImageRecRef img, string_view attribname, // Does it seem to be a float, or did the caller explicitly request // that it be set as a float? p = NULL; - float f = (float)strtod (value.c_str(), &p); + float f = Strutil::strtof (value.c_str(), &p); while (*p && isspace(*p)) ++p; if ((! *p && type == TypeDesc::UNKNOWN) || type == TypeDesc::FLOAT) { @@ -5420,6 +5420,10 @@ main (int argc, char *argv[]) _set_output_format (_TWO_DIGIT_EXPONENT); #endif + // Globally force classic "C" locale, and turn off all formatting + // internationalization, for the entire oiiotool application. + std::locale::global (std::locale::classic()); + ot.imagecache = ImageCache::create (false); ASSERT (ot.imagecache); ot.imagecache->attribute ("forcefloat", 1); diff --git a/src/rla.imageio/rlainput.cpp b/src/rla.imageio/rlainput.cpp index 672ae894c4..126f666b6f 100644 --- a/src/rla.imageio/rlainput.cpp +++ b/src/rla.imageio/rlainput.cpp @@ -392,7 +392,7 @@ RLAInput::seek_subimage (int subimage, int miplevel, ImageSpec &newspec) } } - float aspect = atof (m_rla.AspectRatio); + float aspect = Strutil::stof (m_rla.AspectRatio); if (aspect > 0.f) m_spec.attribute ("PixelAspectRatio", aspect); From 2567bdf0c417dc96d3d58b2c6aa8fba1b52a5b6b Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Wed, 25 Oct 2017 20:41:18 -0700 Subject: [PATCH 2/2] More string fixups -- fix more latent bugs assuming null-terminated string_view More fixes and refactoring after it was pointed out that there were some additional latent bugs related to places where we implicitly assumed that a string_view was null terminated or within a null-terminated string. Also fixed important bugs in Strutil::parse_int and parse_float where it was definitely not safe to assume that the string_view passed was the null-terminated edge of the string, and so, for example if a string_view pointed to the characters "12" but the very next characters were "345" parse_int would return 12345 instead of the correct 12. --- src/include/OpenImageIO/strutil.h | 118 +++++++++++---------- src/include/OpenImageIO/ustring.h | 4 +- src/libutil/filesystem.cpp | 10 +- src/libutil/strutil.cpp | 169 +++++++++++++++++++++++++++--- src/libutil/strutil_test.cpp | 61 ++++++++++- src/oiiotool/oiiotool.cpp | 88 ++++++---------- 6 files changed, 314 insertions(+), 136 deletions(-) diff --git a/src/include/OpenImageIO/strutil.h b/src/include/OpenImageIO/strutil.h index 5b677cee64..85bbe22460 100644 --- a/src/include/OpenImageIO/strutil.h +++ b/src/include/OpenImageIO/strutil.h @@ -290,102 +290,108 @@ float OIIO_API strtof (const char *nptr, char **endptr, const std::locale& loc); -// Helper template to test if a string is a generic type -template -inline bool string_is (string_view /*s*/) { - return false; // Generic: assume there is an explicit specialization -} -// Special case for int -template <> inline bool string_is (string_view s) { - if (s.empty()) - return false; - char *endptr = 0; - strtol (s.data(), &endptr, 10); - return (s.data() + s.size() == endptr); -} -// Special case for float. Note that by using Strutil::strtof, this always -// treats '.' as the decimal character. -template <> inline bool string_is (string_view s) { - if (s.empty()) - return false; - char *endptr = 0; - Strutil::strtof (s.data(), &endptr); - return (s.data() + s.size() == endptr); -} - - - // stoi() returns the int conversion of text from several string types. // No exceptions or errors -- parsing errors just return 0. -inline int stoi (const char* s) { - return s && s[0] ? strtol (s, nullptr, 10) : 0; -} -inline int stoi (const std::string& s) { return Strutil::stoi(s.c_str()); } -inline int stoi (string_view s) { return Strutil::stoi (std::string(s)); } - +OIIO_API int stoi (const char* s, size_t* pos=0, int base=10); +OIIO_API int stoi (const std::string& s, size_t* pos=0, int base=10); +OIIO_API int stoi (string_view s, size_t* pos=0, int base=10); +// N.B. For users of ustring, there's a stoi(ustring) defined in ustring.h. // stoul() returns the unsigned int conversion of text from several string // types. No exceptions or errors -- parsing errors just return 0. -inline unsigned int stoul (const char* s) { - return s && s[0] ? strtoul (s, nullptr, 10) : 0; -} -inline unsigned int stoul (const std::string& s) { return Strutil::stoul(s.c_str()); } -inline unsigned int stoul (string_view s) { return Strutil::stoul (std::string(s)); } - +OIIO_API unsigned int stoul (const char* s, size_t* pos=0, int base=10); +OIIO_API unsigned int stoul (const std::string& s, size_t* pos=0, int base=10); +OIIO_API unsigned int stoul (string_view s, size_t* pos=0, int base=10); +// N.B. For users of ustring, there's a stoi(ustring) defined in ustring.h. /// stof() returns the float conversion of text from several string types. /// No exceptions or errors -- parsing errors just return 0.0. These always /// use '.' for the decimal mark (versus atof and std::strtof, which are /// locale-dependent). -inline float stof (const std::string& s) { - return s.size() ? Strutil::strtof (s.c_str(), nullptr) : 0.0f; -} -inline float stof (const char* s) { - return Strutil::strtof (s, nullptr); -} -inline float stof (string_view s) { - return Strutil::strtof (std::string(s).c_str(), nullptr); -} +OIIO_API float stof (const std::string& s, size_t* pos=0); +OIIO_API float stof (const char* s, size_t* pos=0); +OIIO_API float stof (string_view s, size_t* pos=0); +// N.B. For users of ustring, there's a stof(ustring) defined in ustring.h. // stof() version that takes an explicit locale (for example, if you pass a // default-constructed std::locale, it will use the current native locale's // decimal conventions). -inline float stof (const std::string& s, const std::locale& loc) { - return s.size() ? Strutil::strtof (s.c_str(), nullptr, loc) : 0.0f; +OIIO_API float stof (const std::string& s, const std::locale& loc, size_t* pos=0); +OIIO_API float stof (const char* s, const std::locale& loc, size_t* pos=0); +OIIO_API float stof (string_view s, const std::locale& loc, size_t* pos=0); + + + +/// Return true if the string is exactly (other than leading whitespace) +/// a valid int. +inline bool string_is_int (string_view s) { + size_t pos; + Strutil::stoi (s, &pos); + return pos && pos >= s.size(); // consumed the whole string } -inline float stof (const char* s, const std::locale& loc) { - return Strutil::strtof (s, nullptr, loc); + +/// Return true if the string is exactly (other than leading whitespace) +/// a valid float. This operations in a locale-independent manner, i.e., +/// it assumes '.' as the decimal mark. +inline bool string_is_float (string_view s) { + size_t pos; + Strutil::stof (s, &pos); + return pos && pos >= s.size(); // consumed the whole string } -inline float stof (string_view s, const std::locale& loc) { - return Strutil::strtof (std::string(s).c_str(), nullptr, loc); + +/// Return true if the string is exactly (other than leading whitespace) +/// a valid float. This operations uses an explicit locale. +inline bool string_is_float (string_view s, const std::locale& loc) { + size_t pos; + Strutil::stof (s, loc, &pos); + return pos && pos >= s.size(); // consumed the whole string } // Helper template to convert from generic type to string (using default -// locale). +// locale). Used when you want stoX but you're in a template. template inline T from_string (string_view s) { return T(s); // Generic: assume there is an explicit converter } // Special case for int template<> inline int from_string (string_view s) { - return s.size() ? strtol (s.c_str(), nullptr, 10) : 0; + return Strutil::stoi(s); } // Special case for uint template<> inline unsigned int from_string (string_view s) { - return s.size() ? strtoul (s.c_str(), nullptr, 10) : (unsigned int)0; + return Strutil::stoul(s); } // Special case for float -- note that by using Strutil::strtof, this // always treats '.' as the decimal mark. template<> inline float from_string (string_view s) { - return s.size() ? Strutil::strtof (s.c_str(), nullptr) : 0.0f; + return Strutil::stof(s); } +// Helper template to test if a string is a generic type. Used instead of +// string_is_X, but when you're inside templated code. +template +inline bool string_is (string_view /*s*/) { + return false; // Generic: assume there is an explicit specialization +} +// Special case for int +template <> inline bool string_is (string_view s) { + return string_is_int (s); +} +// Special case for float. Note that by using Strutil::stof, this always +// treats '.' as the decimal character. +template <> inline bool string_is (string_view s) { + return string_is_float (s); +} + + + + /// Given a string containing values separated by a comma (or optionally /// another separator), extract the individual values, placing them into /// vals[] which is presumed to already contain defaults. If only a single diff --git a/src/include/OpenImageIO/ustring.h b/src/include/OpenImageIO/ustring.h index 734795e374..455988232c 100644 --- a/src/include/OpenImageIO/ustring.h +++ b/src/include/OpenImageIO/ustring.h @@ -765,9 +765,9 @@ inline bool iequals (const std::string &a, ustring b) { // add ustring variants of stoi and stof from OpenImageIO/strutil.h namespace Strutil { inline int stoi (ustring s) { return Strutil::stoi (s.c_str()); } -inline int stof (ustring s) { return Strutil::strtof (s.c_str()); } +inline int stof (ustring s) { return Strutil::stof (s.c_str()); } inline int stof (ustring s, const std::locale& loc) { - return Strutil::strtof (s.c_str(), nullptr, loc); + return Strutil::stof (s.c_str(), loc); } } // end namespace Strutil diff --git a/src/libutil/filesystem.cpp b/src/libutil/filesystem.cpp index 8db1142664..cb591a3001 100644 --- a/src/libutil/filesystem.cpp +++ b/src/libutil/filesystem.cpp @@ -723,16 +723,16 @@ Filesystem::enumerate_sequence (string_view desc, std::vector &numbers) // If 'y' is used, generate the complement. std::vector range; Strutil::split (s, range, "-", 2); - int first = Strutil::from_string (range[0]); + int first = Strutil::stoi (range[0]); int last = first; int step = 1; bool complement = false; if (range.size() > 1) { - last = Strutil::from_string (range[1]); + last = Strutil::stoi (range[1]); if (const char *x = strchr (range[1].c_str(), 'x')) - step = (int) strtol (x+1, NULL, 10); + step = Strutil::stoi (x+1); else if (const char *x = strchr (range[1].c_str(), 'y')) { - step = (int) strtol (x+1, NULL, 10); + step = Strutil::stoi (x+1); complement = true; } if (step == 0) @@ -989,7 +989,7 @@ Filesystem::scan_for_matching_filenames(const std::string &pattern_, match_results frame_match; if (regex_match (f, frame_match, pattern_re)) { std::string thenumber (frame_match[1].first, frame_match[1].second); - int frame = (int)strtol (thenumber.c_str(), NULL, 10); + int frame = Strutil::stoi (thenumber); matches.push_back (std::make_pair (frame, f)); } } diff --git a/src/libutil/strutil.cpp b/src/libutil/strutil.cpp index 8aef69b296..463cf1acc9 100644 --- a/src/libutil/strutil.cpp +++ b/src/libutil/strutil.cpp @@ -31,6 +31,7 @@ #include #include +#include #include #include #include @@ -698,14 +699,12 @@ Strutil::parse_int (string_view &str, int &val, bool eat) skip_whitespace (p); if (! p.size()) return false; - const char *end = p.begin(); - int v = strtol (p.begin(), (char**)&end, 10); - if (end == p.begin()) + size_t endpos = 0; + int v = Strutil::stoi (p, &endpos); + if (endpos == 0) return false; // no integer found - if (eat) { - p.remove_prefix (end-p.begin()); - str = p; - } + if (eat) + str = p.substr (endpos); val = v; return true; } @@ -719,14 +718,12 @@ Strutil::parse_float (string_view &str, float &val, bool eat) skip_whitespace (p); if (! p.size()) return false; - const char *end = p.begin(); - float v = Strutil::strtof (p.begin(), (char**)&end); - if (end == p.begin()) + size_t endpos = 0; + float v = Strutil::stof (p, &endpos); + if (endpos == 0) return false; // no integer found - if (eat) { - p.remove_prefix (end-p.begin()); - str = p; - } + if (eat) + str = p.substr (endpos); val = v; return true; } @@ -1183,4 +1180,148 @@ Strutil::strtod (const char *nptr, char **endptr) // cppreference on locale: // http://en.cppreference.com/w/cpp/locale/locale + + +int +Strutil::stoi (const char* s, size_t* pos, int base) +{ + if (s) { + char* endptr; + int r = strtol (s, &endptr, base); + if (endptr != s) { + if (pos) + *pos = size_t (endptr - s); + return r; + } + } + // invalid + if (pos) + *pos = 0; + return 0; +} + + +int +Strutil::stoi (const std::string& s, size_t* pos, int base) +{ + return Strutil::stoi(s.c_str(), pos, base); +} + + +int +Strutil::stoi (string_view s, size_t* pos, int base) +{ + // string_view may not be ended with a terminating null, so for safety, + // create a temporary string. + return Strutil::stoi (std::string(s), pos, base); +} + + + +unsigned int +Strutil::stoul (const char* s, size_t* pos, int base) +{ + if (s) { + char* endptr; + unsigned int r = strtoul (s, &endptr, base); + if (endptr != s) { + if (pos) + *pos = size_t (endptr - s); + return r; + } + } + // invalid + if (pos) + *pos = 0; + return 0; +} + + +unsigned int +Strutil::stoul (const std::string& s, size_t* pos, int base) +{ + return Strutil::stoul(s.c_str(), pos, base); +} + + +unsigned int +Strutil::stoul (string_view s, size_t* pos, int base) +{ + // string_view may not be ended with a terminating null, so for safety, + // create a temporary string. + return Strutil::stoul (std::string(s), pos, base); +} + + + +float +Strutil::stof (const char* s, size_t* pos) +{ + if (s) { + char* endptr; + float r = Strutil::strtof (s, &endptr); + if (endptr != s) { + if (pos) + *pos = size_t (endptr - s); + return r; + } + } + // invalid + if (pos) + *pos = 0; + return 0; +} + + +float +Strutil::stof (const std::string& s, size_t* pos) +{ + return Strutil::stof(s.c_str(), pos); +} + + +float +Strutil::stof (string_view s, size_t* pos) +{ + // string_view may not be ended with a terminating null, so for safety, + // create a temporary string. + return Strutil::stof (std::string(s), pos); +} + + + +float +Strutil::stof (const char* s, const std::locale& loc, size_t* pos) +{ + if (s) { + char* endptr; + float r = Strutil::strtof (s, &endptr, loc); + if (endptr != s) { + if (pos) + *pos = size_t (endptr - s); + return r; + } + } + // invalid + if (pos) + *pos = 0; + return 0; +} + + +float +Strutil::stof (const std::string& s, const std::locale& loc, size_t* pos) +{ + return Strutil::stof(s.c_str(), loc, pos); +} + + +float +Strutil::stof (string_view s, const std::locale& loc, size_t* pos) +{ + // string_view may not be ended with a terminating null, so for safety, + // create a temporary string. + return Strutil::stof (std::string(s), loc, pos); +} + OIIO_NAMESPACE_END diff --git a/src/libutil/strutil_test.cpp b/src/libutil/strutil_test.cpp index 380fc69c61..50837ea63b 100644 --- a/src/libutil/strutil_test.cpp +++ b/src/libutil/strutil_test.cpp @@ -354,6 +354,27 @@ void test_replace () void test_conversion () { std::cout << "Testing string_is, string_from conversions\n"; + size_t pos; + + OIIO_CHECK_EQUAL (Strutil::string_is_int("142"), true); + OIIO_CHECK_EQUAL (Strutil::string_is_int("142.0"), false); + OIIO_CHECK_EQUAL (Strutil::string_is_int(""), false); + OIIO_CHECK_EQUAL (Strutil::string_is_int(" "), false); + OIIO_CHECK_EQUAL (Strutil::string_is_int("foo"), false); + OIIO_CHECK_EQUAL (Strutil::string_is_int("142x"), false); + OIIO_CHECK_EQUAL (Strutil::string_is_int(" 142"), true); + OIIO_CHECK_EQUAL (Strutil::string_is_int("x142"), false); + + OIIO_CHECK_EQUAL (Strutil::string_is_float("142"), true); + OIIO_CHECK_EQUAL (Strutil::string_is_float("142.0"), true); + OIIO_CHECK_EQUAL (Strutil::string_is_float(""), false); + OIIO_CHECK_EQUAL (Strutil::string_is_float(" "), false); + OIIO_CHECK_EQUAL (Strutil::string_is_float("foo"), false); + OIIO_CHECK_EQUAL (Strutil::string_is_float("142x"), false); + OIIO_CHECK_EQUAL (Strutil::string_is_float(" 142"), true); + OIIO_CHECK_EQUAL (Strutil::string_is_float("x142"), false); + + // Note that string_is<> is DEPREATED as of OIIO 1.9. OIIO_CHECK_EQUAL (Strutil::string_is("142"), true); OIIO_CHECK_EQUAL (Strutil::string_is("142.0"), false); OIIO_CHECK_EQUAL (Strutil::string_is(""), false); @@ -361,7 +382,6 @@ void test_conversion () OIIO_CHECK_EQUAL (Strutil::string_is("142x"), false); OIIO_CHECK_EQUAL (Strutil::string_is(" 142"), true); OIIO_CHECK_EQUAL (Strutil::string_is("x142"), false); - OIIO_CHECK_EQUAL (Strutil::string_is("142"), true); OIIO_CHECK_EQUAL (Strutil::string_is("142.0"), true); OIIO_CHECK_EQUAL (Strutil::string_is(""), false); @@ -371,39 +391,74 @@ void test_conversion () OIIO_CHECK_EQUAL (Strutil::string_is("x142"), false); OIIO_CHECK_EQUAL (Strutil::stoi("hi"), 0); + OIIO_CHECK_EQUAL (Strutil::stoi(" "), 0); OIIO_CHECK_EQUAL (Strutil::stoi("123"), 123); OIIO_CHECK_EQUAL (Strutil::stoi("-123"), -123); OIIO_CHECK_EQUAL (Strutil::stoi(" 123 "), 123); OIIO_CHECK_EQUAL (Strutil::stoi("123.45"), 123); + OIIO_CHECK_EQUAL (Strutil::stoi("hi", &pos), 0); + OIIO_CHECK_EQUAL (pos, 0); + OIIO_CHECK_EQUAL (Strutil::stoi(" ", &pos), 0); + OIIO_CHECK_EQUAL (pos, 0); + OIIO_CHECK_EQUAL (Strutil::stoi("123", &pos), 123); + OIIO_CHECK_EQUAL (pos, 3); + OIIO_CHECK_EQUAL (Strutil::stoi("-123", &pos), -123); + OIIO_CHECK_EQUAL (pos, 4); + OIIO_CHECK_EQUAL (Strutil::stoi(" 123 ", &pos), 123); + OIIO_CHECK_EQUAL (pos, 4); + OIIO_CHECK_EQUAL (Strutil::stoi("123.45", &pos), 123); + OIIO_CHECK_EQUAL (pos, 3); + OIIO_CHECK_EQUAL (Strutil::stoul("hi"), unsigned(0)); + OIIO_CHECK_EQUAL (Strutil::stoul(" "), unsigned(0)); OIIO_CHECK_EQUAL (Strutil::stoul("123"), unsigned(123)); OIIO_CHECK_EQUAL (Strutil::stoul("-123"), unsigned(-123)); OIIO_CHECK_EQUAL (Strutil::stoul(" 123 "), unsigned(123)); OIIO_CHECK_EQUAL (Strutil::stoul("123.45"), unsigned(123)); OIIO_CHECK_EQUAL (Strutil::stof("hi"), 0.0f); + OIIO_CHECK_EQUAL (Strutil::stof(" "), 0.0f); OIIO_CHECK_EQUAL (Strutil::stof("123"), 123.0f); OIIO_CHECK_EQUAL (Strutil::stof("-123"), -123.0f); OIIO_CHECK_EQUAL (Strutil::stof("123.45"), 123.45f); + OIIO_CHECK_EQUAL (Strutil::stof("123.45xyz"), 123.45f); OIIO_CHECK_EQUAL (Strutil::stof(" 123.45 "), 123.45f); OIIO_CHECK_EQUAL (Strutil::stof("123.45+12"), 123.45f); OIIO_CHECK_EQUAL (Strutil::stof("1.2345e+2"), 123.45f); + + OIIO_CHECK_EQUAL (Strutil::stof("hi", &pos), 0.0f); + OIIO_CHECK_EQUAL (pos, 0); + OIIO_CHECK_EQUAL (Strutil::stof(" ", &pos), 0.0f); + OIIO_CHECK_EQUAL (pos, 0); + OIIO_CHECK_EQUAL (Strutil::stof("123", &pos), 123.0f); + OIIO_CHECK_EQUAL (pos, 3); + OIIO_CHECK_EQUAL (Strutil::stof("-123", &pos), -123.0f); + OIIO_CHECK_EQUAL (pos, 4); + OIIO_CHECK_EQUAL (Strutil::stof("123.45", &pos), 123.45f); + OIIO_CHECK_EQUAL (pos, 6); + OIIO_CHECK_EQUAL (Strutil::stof("123.45xyz", &pos), 123.45f); + OIIO_CHECK_EQUAL (pos, 6); + OIIO_CHECK_EQUAL (Strutil::stof(" 123.45 ", &pos), 123.45f); + OIIO_CHECK_EQUAL (pos, 7); + OIIO_CHECK_EQUAL (Strutil::stof("123.45+12", &pos), 123.45f); + OIIO_CHECK_EQUAL (pos, 6); + OIIO_CHECK_EQUAL (Strutil::stof("1.2345e2", &pos), 123.45f); + OIIO_CHECK_EQUAL (pos, 8); // stress case! OIIO_CHECK_EQUAL (Strutil::stof("100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001E-200"), 1.0f); + // Note that from_string<> is DEPREATED as of OIIO 1.9. OIIO_CHECK_EQUAL (Strutil::from_string("hi"), 0); OIIO_CHECK_EQUAL (Strutil::from_string("123"), 123); OIIO_CHECK_EQUAL (Strutil::from_string("-123"), -123); OIIO_CHECK_EQUAL (Strutil::from_string(" 123 "), 123); OIIO_CHECK_EQUAL (Strutil::from_string("123.45"), 123); - OIIO_CHECK_EQUAL (Strutil::from_string("hi"), unsigned(0)); OIIO_CHECK_EQUAL (Strutil::from_string("123"), unsigned(123)); OIIO_CHECK_EQUAL (Strutil::from_string("-123"), unsigned(-123)); OIIO_CHECK_EQUAL (Strutil::from_string(" 123 "), unsigned(123)); OIIO_CHECK_EQUAL (Strutil::from_string("123.45"), unsigned(123)); - OIIO_CHECK_EQUAL (Strutil::from_string("hi"), 0.0f); OIIO_CHECK_EQUAL (Strutil::from_string("123"), 123.0f); OIIO_CHECK_EQUAL (Strutil::from_string("-123"), -123.0f); diff --git a/src/oiiotool/oiiotool.cpp b/src/oiiotool/oiiotool.cpp index adad15ec67..66d66648af 100644 --- a/src/oiiotool/oiiotool.cpp +++ b/src/oiiotool/oiiotool.cpp @@ -1325,34 +1325,20 @@ set_input_attribute (int argc, const char *argv[]) return 0; } - // Does it seem to be an int, or did the caller explicitly request - // that it be set as an int? - char *p = NULL; - int i = strtol (value.c_str(), &p, 10); - while (*p && isspace(*p)) - ++p; - if ((! *p && type == TypeDesc::UNKNOWN) || type == TypeDesc::INT) { - // int conversion succeeded and accounted for the whole string -- - // so set an int attribute. - ot.input_config.attribute (attribname, i); - return 0; - } - - // Does it seem to be a float, or did the caller explicitly request - // that it be set as a float? - p = NULL; - float f = Strutil::strtof (value.c_str(), &p); - while (*p && isspace(*p)) - ++p; - if ((! *p && type == TypeDesc::UNKNOWN) || type == TypeDesc::FLOAT) { - // float conversion succeeded and accounted for the whole string -- - // so set a float attribute. - ot.input_config.attribute (attribname, f); - return 0; + if (type == TypeInt || + (type == TypeUnknown && Strutil::string_is_int(value))) { + // Does it seem to be an int, or did the caller explicitly request + // that it be set as an int? + ot.input_config.attribute (attribname, Strutil::stoi(value)); + } else if (type == TypeFloat || + (type == TypeUnknown && Strutil::string_is_float(value))) { + // Does it seem to be a float, or did the caller explicitly request + // that it be set as a float? + ot.input_config.attribute (attribname, Strutil::stof(value)); + } else { + // Otherwise, set it as a string attribute + ot.input_config.attribute (attribname, value); } - - // Otherwise, set it as a string attribute - ot.input_config.attribute (attribname, value); return 0; } @@ -1478,38 +1464,28 @@ OiioTool::set_attribute (ImageRecRef img, string_view attribname, return true; } - // Does it seem to be an int, or did the caller explicitly request - // that it be set as an int? - char *p = NULL; - int i = strtol (value.c_str(), &p, 10); - while (*p && isspace(*p)) - ++p; - if ((! *p && type == TypeDesc::UNKNOWN) || type == TypeDesc::INT) { - // int conversion succeeded and accounted for the whole string -- - // so set an int attribute. + if (type == TypeInt || + (type == TypeUnknown && Strutil::string_is_int(value))) { + // Does it seem to be an int, or did the caller explicitly request + // that it be set as an int? + int v = Strutil::stoi(value); return apply_spec_mod (*img, do_set_any_attribute, - std::pair(attribname,i), + std::pair(attribname,v), allsubimages); - } - - // Does it seem to be a float, or did the caller explicitly request - // that it be set as a float? - p = NULL; - float f = Strutil::strtof (value.c_str(), &p); - while (*p && isspace(*p)) - ++p; - if ((! *p && type == TypeDesc::UNKNOWN) || type == TypeDesc::FLOAT) { - // float conversion succeeded and accounted for the whole string -- - // so set a float attribute. + } else if (type == TypeFloat || + (type == TypeUnknown && Strutil::string_is_float(value))) { + // Does it seem to be a float, or did the caller explicitly request + // that it be set as a float? + float v = Strutil::stof(value); return apply_spec_mod (*img, do_set_any_attribute, - std::pair(attribname,f), + std::pair(attribname,v), + allsubimages); + } else { + // Otherwise, set it as a string attribute + return apply_spec_mod (*img, do_set_any_attribute, + std::pair(attribname,value), allsubimages); } - - // Otherwise, set it as a string attribute - return apply_spec_mod (*img, do_set_any_attribute, - std::pair(attribname,value), - allsubimages); } @@ -3789,7 +3765,7 @@ action_mosaic (int argc, const char *argv[]) std::map options; options["pad"] = "0"; ot.extract_options (options, command); - int pad = strtol (options["pad"].c_str(), NULL, 10); + int pad = Strutil::stoi (options["pad"]); ImageSpec Rspec (ximages*widest + (ximages-1)*pad, yimages*highest + (yimages-1)*pad, @@ -4016,7 +3992,7 @@ action_clamp (int argc, const char *argv[]) ot.extract_options (options, command); Strutil::extract_from_list_string (min, options["min"]); Strutil::extract_from_list_string (max, options["max"]); - bool clampalpha01 = strtol (options["clampalpha"].c_str(), NULL, 10) != 0; + bool clampalpha01 = Strutil::stoi (options["clampalpha"]); for (int m = 0, miplevels=R->miplevels(s); m < miplevels; ++m) { ImageBuf &Rib ((*R)(s,m));