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.

Author: lgritz

.travis.yml

@@ -33,6 +33,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,35 +97,54 @@ 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)
 {
     return tinyformat::format (fmt.c_str(), args...);
 }
 
+/// A version of Strutil::format() that uses an explicit locale.
+template<typename... Args>
+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<typename... Args>
 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<typename... Args>
+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<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 honor the existing locale conventions of
+/// the ostream.
 template<typename... Args>
 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<typename T>
 inline bool string_is (string_view /*s*/) {
@@ -267,33 +303,85 @@ template <> inline bool string_is<int> (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<float> (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<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 s.size() ? strtol (s.c_str(), nullptr, 10) : 0;
 }
 // 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 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<float> (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;
 }
 
 

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,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<typename... Args>
     static ustring format (string_view fmt, const Args&... args)
     {
         return ustring (Strutil::format (fmt, args...));
     }
 
+    /// ustring formatting with an explicit locale.
+    template<typename... Args>
+    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

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

@@ -1389,6 +1389,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) {
@@ -1420,6 +1421,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);
@@ -1439,6 +1441,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 (";
         {