More principled handling of string<->numeric conversions.

This refactor comes after a belated realization that our numeric
conversion routines (principally from_string<> and format) 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.

After much debate, and a few implementations, we are declaring that
since the whole point of OIIO is about reading and writing persistent
data (image files!), that in order to be consistent, ALL of OIIO's
string parsing and formatting, including that done by utility library
functions, will be locale-independent by using the "C" locale (in
particular, we will use the usual North American default of '.' for
decimal separator).

Additionally:

* Added a new stof(), stoi(), stoul() to replace the clunkier template
syntax of from_string<>. As explained, these are hard-coded to "C" locale,
not the native/global locale.

* Much more attention to string_view safety, finding a number of subtle
bugs (including Strutil::parse_int, etc.)  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.

* 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.
We may revise this later depending on what the upstream tinyformat
chooses to do about the issue.

* 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.

If we ever decide that we want versions of these functions that take
explicit locales, it's not hard to add.

Author: lgritz

.travis.yml

@@ -32,6 +32,7 @@ addons:
       - libavformat-dev
       - libswscale-dev
       - libavutil-dev
+      - locales
       # - qtbase5-dev   # FIXME: enable Qt5 on Linux
       # - bzip2
       # - libtinyxml-dev

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

src/include/OpenImageIO/optparser.h

@@ -40,6 +40,7 @@
 #define OPENIMAGEIO_OPTPARSER_H
 
 #include <string>
+#include <OpenImageIO/strutil.h>
 
 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
 

src/include/OpenImageIO/strutil.h

@@ -97,6 +97,8 @@ 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<typename... Args>
 inline std::string format (string_view fmt, const Args&... args)
 {
@@ -105,23 +107,25 @@ inline std::string format (string_view fmt, const Args&... 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<typename... Args>
 inline void printf (string_view fmt, const Args&... args)
 {
     sync_output (stdout, format(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<typename... Args>
 inline void fprintf (FILE *file, string_view fmt, const Args&... args)
 {
     sync_output (file, format(fmt, args...));
 }
 
+
 /// Output formatted string to an open ostream, type-safe, and threads can't
-/// clobber one another.
+/// clobber one another. This will force classic "C" locale conventions.
 template<typename... Args>
 inline void fprintf (std::ostream &file, string_view fmt, const Args&... args)
 {
@@ -254,50 +258,99 @@ 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);
 
-// Helper template to test if a string is a generic type
-template<typename T>
-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<int> (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
-template <> inline bool string_is<float> (string_view s) {
-    if (s.empty())
-        return false;
-    char *endptr = 0;
-    strtod (s.data(), &endptr);
-    return (s.data() + s.size() == endptr);
+
+/// 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);
+
+
+// stoi() returns the int conversion of text from a string.
+// No exceptions or errors -- parsing errors just return 0, over/underflow
+// gets clamped to int range. No locale consideration.
+OIIO_API int stoi (string_view s, size_t* pos=0, int base=10);
+
+// stoui() returns the unsigned int conversion of text from a string.
+// No exceptions or errors -- parsing errors just return 0. Negative
+// values are cast, overflow is clamped. No locale considerations.
+inline unsigned int stoui (string_view s, size_t* pos=0, int base=10) {
+    return static_cast<unsigned int>(stoi (s, pos, base));
 }
 
+/// 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).
+OIIO_API float stof (string_view s, size_t* pos=0);
+#define OIIO_STRUTIL_HAS_STOF 1  /* be able to test this */
+
+// Temporary fix: allow separate std::string and char* versions, to avoid
+// string_view allocation on some platforms. This will be deprecated once
+// we can count on all supported compilers using short string optimization.
+OIIO_API float stof (const std::string& s, size_t* pos=0);
+OIIO_API float stof (const char* s, size_t* pos=0);
+// N.B. For users of ustring, there's a stof(ustring) defined in ustring.h.
+
+OIIO_API double stod (string_view s, size_t* pos=0);
+OIIO_API double stod (const std::string& s, size_t* pos=0);
+OIIO_API double stod (const char* s, size_t* pos=0);
+
 
 
-// Helper template to convert from generic type to string
+/// Return true if the string is exactly (other than leading  and trailing
+/// whitespace) a valid int.
+OIIO_API bool string_is_int (string_view s);
+
+/// Return true if the string is exactly (other than leading or trailing
+/// whitespace) a valid float. This operations in a locale-independent
+/// manner, i.e., it assumes '.' as the decimal mark.
+OIIO_API bool string_is_float (string_view s);
+
+
+
+// Helper template to convert from generic type to string. Used when you
+// want stoX but you're in a template. Rigged to use "C" locale.
 template<typename T>
 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<int> (string_view s) {
-    return s.size() ? strtol (s.c_str(), NULL, 10) : 0;
+    return Strutil::stoi(s);
 }
 // Special case for uint
 template<> inline unsigned int from_string<unsigned int> (string_view s) {
-    return s.size() ? strtoul (s.c_str(), NULL, 10) : (unsigned int)0;
+    return Strutil::stoui(s);
 }
-// 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<float> (string_view s) {
-    return s.size() ? (float)strtod (s.c_str(), NULL) : 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<typename T>
+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<int> (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<float> (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

src/include/OpenImageIO/tinyformat.h

@@ -266,6 +266,7 @@ template<typename T>
 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<int>(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,23 +946,40 @@ 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<typename... Args>
 void format(std::ostream& out, const char* fmt, const Args&... args)
 {
     vformat(out, fmt, makeFormatList(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<typename... Args>
 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<typename... Args>
+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<typename... Args>
 void printf(const char* fmt, const Args&... args)
 {
@@ -1011,6 +1031,16 @@ template<TINYFORMAT_ARGTYPES(n)>                                          \
 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<TINYFORMAT_ARGTYPES(n)>                                          \
+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();                                                     \
 }                                                                         \

src/include/OpenImageIO/ustring.h

@@ -626,6 +626,8 @@ 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<typename... Args>
     static ustring format (string_view fmt, const Args&... args)
     {
@@ -751,6 +753,12 @@ inline bool iequals (const std::string &a, ustring b) {
 }
 
 
+
+// ustring variant stof from OpenImageIO/strutil.h
+namespace Strutil {
+inline int stof (ustring s) { return Strutil::stof (s.string()); }
+} // end namespace Strutil
+
 OIIO_NAMESPACE_END
 
 #endif // OPENIMAGEIO_USTRING_H

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 

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();
 }

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);

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 (";
         {