<div dir="ltr"><div>The majority of the changes on the string-convert branch have now been merged</div><div>onto master.</div><div><br></div><div>The changes that are left to commit are:</div><div><br></div><div>- Removal of the LocaleGuards that are no longer necessary (nearly all of them)</div><div> which should have no impact now that numeric conversion is independent of </div><div> locale other than a slight benefit to performance.</div><div><br></div><div>- Resetting the C and C++ locales to the users preferred locale in LocaleGuard</div><div><span style="white-space:pre"> </span>destructor so that the C++ locale is not always set to C/POSIX. This change</div><div><span style="white-space:pre"> </span>will mean that theoretically we can use the C and C++ API's to generate</div><div><span style="white-space:pre"> </span>localized strings for display. In reality the compiler/library support for</div><div><span style="white-space:pre"> </span>C++ locales on non-linux platforms means we should probably avoid using C++</div><div><span style="white-space:pre"> </span>API(iostream) for localization, at least for now.</div><div><br></div><div>Both changes *should* not cause any change to current behaviour but to reduce</div><div>the amount of change/possible bugs I'll wait until the next release cycle to</div><div>apply those.</div><div><br></div><div>So basically from now, where any numeric conversion that is required for</div><div>de/serialization or where numeric conversion is required to be the the "C"</div><div>locale the API defined in pbd/string_convert.h should be used (generally via</div><div>XMLNode::get/set_property) or conversely only use snprinf/sscanf,</div><div>std::stringstream or PBD::string_compose for numeric conversion where</div><div>localization is required.</div><div><br></div><div>If any issues are found please create a report in the bug tracker with</div><div>Session/config files attached and I'll deal with it ASAP.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 4, 2017 at 10:52 PM, Tim Mayberry <span dir="ltr"><<a href="mailto:mojofunk@gmail.com" target="_blank">mojofunk@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>I've pushed a branch called string-convert to the main repository that I'd like </div><div>some feedback on from the other developers when they get a chance. It </div><div>represents a fair bit of work over the last year or so and is attempt to make </div><div>improvements to the de/serialization code in Ardour.</div><div><br></div><div>There may be a few further tweaks necessary but I feel it is ready to merge (</div><div>assuming there are no issues with that) at an appropriate time.</div><div><br></div><div>The changes result in some backwards compatible modifications to the Session</div><div>file format and config files, so with that in mind it may be a candidate as a</div><div>"feature" for the next major version and an increase in the Session file format</div><div>version may be a good idea (although perhaps not necessary).</div><div><br></div><div>To summarize the implementation, I've added a new C++ API in libpbd used for</div><div>converting types <-> strings for built in types and for other types via</div><div>template specialization. The API is intended to be thread safe and all</div><div>conversions are performed as if in the C locale without changing the global C</div><div>or C++ locales. Some requirements of the API were to ensure round-trip</div><div>equality, reduce complexity, improve readability and increase the amount of</div><div>error checking/logging.</div><div><br></div><div>The new API is then used in the PBD::XMLNode, PBD::Properties and</div><div>GUIObjectState classes and elsewhere to provide the above benefits and try to</div><div>ensure there is only a single implementation for each type <-> string</div><div>conversion. As a result the LocaleGuards that are used to guard internal string</div><div>conversions are no longer necessary and are removed.</div><div><br></div><div>What follows is some background information and implementation details for </div><div>those that are interested.</div><div><br></div><div># I18n and Serialization</div><div><br></div><div>An application like Ardour that has native language support will generally use</div><div>a users preferred locale setting from their system environment to determine</div><div>what language is used and aspects like formatting of numeric values.</div><div><br></div><div>Ardour uses the GNU gettext utilities and runtime library for retrieval of</div><div>translated messages for display in the GUI. Ardour is written primarily in C++,</div><div>so for locale specific formatting of numeric input and output, functionality</div><div>from either the C standard library(sscanf/snprintf) or C++ standard</div><div>library(iostreams) would normally be used.</div><div><br></div><div>When the data that is part of the application state, like Session and</div><div>configuration data gets converted from an in memory binary format to a</div><div>string/character based format for serialization to disk, the numeric data</div><div>should not be formatted in a locale specific format as it makes correct parsing</div><div>dependent on using that locale. The main issue is some locales use a comma to</div><div>mark the decimal point in floating point numbers(1.25 vs 1,25) or use grouping</div><div>of integral numbers(1,200 vs 1200), so to avoid this all numeric data is</div><div>formatted using the same locale, the "C" locale.</div><div><br></div><div>To perform numeric conversion to and from numeric types with the "C" locale</div><div>using the standard scanf/printf family of functions the locale has to be</div><div>changed from the current locale (usually that is the users preferred locale set</div><div>at the start of program execution to support NLS/localization) using</div><div>setlocale() and then back again after numeric conversion.</div><div><br></div><div>C and C++ use different data structures/classes to represent locale data. It is</div><div>possible that the C and C++ API's have a different "setting" for the current</div><div>global locale and slightly confusingly setting the C++ locale sets the C and</div><div>C++ locale but setting the C locale only sets the C locale and not the C++</div><div>locale.</div><div><br></div><div>The standard C++ library supports the concept of imbuing (via std::ios::imbue)</div><div>an iostream with a locale so that a specific locale is used when converting</div><div>numeric data to a string or vica verca. Unfortunately this has not been well</div><div>supported by C/C++ compilers/libraries and when I recently tested it did not</div><div>work with all the platforms/options/compilers we use and is not currently used</div><div>in the Ardour codebase.</div><div><br></div><div>Also, changing the global C and or C++ locale is not thread-safe as it modifies</div><div>shared global data which may be used from various C/C++ run-time functions,</div><div>such as error messages, time/date, translation (gettext), floating point</div><div>numbers formatting (printf/iostream) and parsing (scanf/iostream), character</div><div>types (<ctype.h>) and wide to multi-byte characters conversion functions.</div><div><br></div><div>This means that if one thread is accessing the global C locale while another is</div><div>modifying the locale with ::setlocale it may lead to a crash on some systems</div><div>(Windows and at least some versions of OS X). This would not normally be an</div><div>issue if the application ensures that only a single thread uses API's that</div><div>access global locale data. The GUI thread usually needs to use the C and or C++</div><div>API's for localization so if the serialization code uses those same API's then</div><div>that has to be taken into consideration. If the serialization code is executed</div><div>in the GUI thread as it is in Ardour currently then there is not a thread</div><div>safety issue. I think the only case currently where it may be an issue is when</div><div>executing arbitrary code loaded via plugins/loadable modules in another thread. </div><div><br></div><div><br></div><div># Current issues?</div><div><br></div><div>## Round Trip Equality</div><div><br></div><div>Round trip equality meaning to convert data from a binary format to a</div><div>string/character representation for serialization and then back again will</div><div>result in data that is considered to be equal to the value of the data before</div><div>conversion.</div><div><br></div><div>This is mostly relevant for floating point values that (in the worst case) need</div><div>to be stored/serialized with a certain level of precision (9 decimal digits for</div><div>float, 17 for double) so that the state restoration/deserialization results in</div><div>an equality test passing. i.e</div><div><span class="m_172317899430093833m_1770041226664996164gmail-Apple-tab-span" style="white-space:pre-wrap"> </span></div><div><span class="m_172317899430093833m_1770041226664996164gmail-Apple-tab-span" style="white-space:pre-wrap"> </span>double d = random_double();</div><div><span class="m_172317899430093833m_1770041226664996164gmail-Apple-tab-span" style="white-space:pre-wrap"> </span>assert (d == string_to<double> (to_string<double> (d)));</div><div><br></div><div>Most of the floating point values that are stored as part of Ardour's state</div><div>data do not use enough precision to pass a round trip equality test. I think</div><div>this is sub-optimal.</div><div><br></div><div>## Complexity</div><div><br></div><div>Using API's like snprinf/sscanf that rely on the global locale mean using</div><div>LocaleGuards to ensure that conversions like float <-> string are performed in</div><div>the "C" locale. You have to remember to add a LocaleGuard in the scope that the</div><div>conversion occurs as you can not be sure of the current setting of the global</div><div>locale. There have been a number of bugs fixed in Ardour caused by issue over</div><div>time.</div><div><br></div><div>If a C++ class like std::stringstream is used to perform conversion then you</div><div>would normally have to ensure that the global C++ locale was set to "C" for</div><div>both float and integer conversions as decimal grouping may also be performed</div><div>for numeric formatting of integers with the C++ API.</div><div><br></div><div>## Thread Safety</div><div><br></div><div>As mentioned above, changing the global C and C++ locales is not thread safe. As</div><div>most DAW's including Ardour support loading plugins and we don't control what</div><div>code is executed, it is possible (although unlikely) for a plugin to access the</div><div>global C or C++ locale while we are changing the global locale and cause a</div><div>crash.</div><div><br></div><div>There are options on some platforms to have per thread global locales but it is</div><div>not a portable solution (or a very good one IMO). There are also functions on</div><div>some platforms like strtod_l and strtof_l that take a locale as a parameter and</div><div>are thread safe.</div><div><br></div><div>## Error checking</div><div><br></div><div>If you look at the use of the C and C++ API's for serialization in the Ardour</div><div>code base there is a lack of error checking. So any solution would hopefully</div><div>improve this situation and allow for any errors to at least be identified.</div><div><br></div><div>## Performance</div><div><br></div><div>Not a major issue but changing the global locale on some platforms (at least</div><div>Windows) can be a slow operation and some changes have been introduced in the</div><div>past to mitigate this.</div><div><br></div><div><br></div><div># Implementation</div><div><br></div><div>The changes introduce a simple C++ based API in the form of PBD::to_string()</div><div>and PBD::string_to() template functions and conversion functions for the</div><div>builtin types(bool,float,int32_t etc) which are declared in the</div><div>pbd/string_convert.h header.</div><div><br></div><div>The current implementation of the functions used to convert the builtin types</div><div>use snprintf/sscanf for integer types, the glib function g_ascii_strtod and</div><div>g_ascii_dtostr for float/double and simple code for bool.</div><div><br></div><div>To simplify creating template specializations of to_string/string_to for enum</div><div>types registered with the PBD::EnumWriter class, a simple macro</div><div>DEFINE_ENUM_CONVERT is defined in pbd/types_convert.h. This replaces the need</div><div>for i/ostream& operator>>/<<() for instance in ardour/types.h</div><div><br></div><div>Many of the changes are related to changing the interface of XMLNode for</div><div>setting properties which is used by much of the serialization code. The</div><div>current XMLNode::property/add_property interface is mostly replaced by a new</div><div>template based XMLNode::get/set_property interface that takes the name of the</div><div>property and a reference to the variable to be de/serialized to/from and then</div><div>internalizes the type <-> string conversion.</div><div><br></div><div>For instance in the case of setting an XMLNode property:</div><div><br></div><div> snprintf (buf, sizeof (buf), "%.12g", _min_yval);</div><div> root->add_property ("min-yval", buf);</div><div><br></div><div>Becomes:</div><div><br></div><div> root->set_property ("min-yval", _min_yval);</div><div><br></div><div>Where XMLNode::set_property is defined as:</div><div><br></div><div>template<class T></div><div>bool set_property (const char* name, const T& value)</div><div>{</div><div> std::string str;</div><div> if (!PBD::to_string<T> (value, str)) {</div><div> return false;</div><div> }</div><div> return set_property(name, str);</div><div>}</div><div><br></div><div>And for getting the same property from an XMLNode:</div><div><br></div><div>if ((prop = node.property (X_("min-yval"))) != 0) {</div><div> _min_yval = atof (prop->value ().c_str());</div><div>} else {</div><div> _min_yval = FLT_MIN;</div><div>}</div><div><br></div><div>Becomes:</div><div><br></div><div>if (!node.get_property (X_("min-yval"), _min_yval)) {</div><div> _min_yval = FLT_MIN;</div><div>}</div><div><br></div><div>Where XMLNode::get_property is defined as:</div><div><br></div><div>template <class T></div><div>bool get_property (const char* name, T& value) const</div><div>{</div><div> XMLProperty const* const prop = property (name);</div><div> if (!prop) {</div><div> return false;</div><div> }</div><div><br></div><div> if (!PBD::string_to<T>(prop->valu<wbr>e (), value)) {</div><div> return false;</div><div> }</div><div> return true;</div><div>}</div><div><br></div><div>Which has the advantage in that it is checking that the property exists and</div><div>that the string conversion was successful as you very rarely need to do them</div><div>separately.</div><div><br></div><div>The other main type of change in the branch is to replace all direct and</div><div>indirect (via PBD::string_compose) use of std::stringstream for serialization</div><div>so that the correct numeric conversion is not dependent on a LocaleGuard/global</div><div>locale.</div><div><br></div><div>As std::i/ostream derived classes by default use the global locale for numeric</div><div>conversion and currently the global C++ locale is always set to use the "C"</div><div>locale via PBD::LocaleGuard::LocaleGuard (and not reset in dtor).</div><div><br></div><div>If it is intended that the global C++ locale is set to the users preferred</div><div>locale and is used for conversion of numeric data to strings for display then</div><div>perhaps the string_compose template should be renamed/aliased as something like</div><div>i18n_compose/i18n_fmt to remind the user of the API that any numeric data types</div><div>that are passed as arguments may be converted to a localized format and the API</div><div>is not appropriate to use for serialization.</div><div><br></div><div>In terms of testing performance of the new code I haven't done much conclusive</div><div>testing but from what I have done on Linux there is a reduction in the time</div><div>required to save a session of about 25%.</div><div><br></div><div># Examples of Session/Config file changes</div><div><br></div><div>## Enums are saved as literal strings rather than numbers.</div><div><br></div><div>So for instance:</div><div><br></div><div> <Option name="monitoring-model" value="2"/></div><div> <Option name="meter-type-master" value="16"/></div><div><br></div><div>Becomes:</div><div><br></div><div> <Option name="monitoring-model" value="ExternalMonitoring"/></div><div> <Option name="meter-type-master" value="MeterK20"/></div><div><br></div><div>## The string representation of all boolean values is the same</div><div><br></div><div>At the moment this is yes/no which I think helps distiguish boolean values from</div><div>numeric but it could easily be changed so they are 0/1 or whatever.</div><div><br></div><div>So for instance:</div><div><br></div><div> <Option name="use-region-fades" value="1"/></div><div> <Option name="use-transport-fades" value="1"/></div><div> <Option name="use-monitor-fades" value="1"/></div><div> <Option name="native-file-data-format" value="0"/></div><div> <Option name="native-file-header-forma<wbr>t" value="1"/></div><div> <Option name="auto-play" value="0"/></div><div><br></div><div>Becomes:</div><div><br></div><div> <Option name="use-region-fades" value="yes"/></div><div> <Option name="use-transport-fades" value="yes"/></div><div> <Option name="use-monitor-fades" value="yes"/></div><div> <Option name="native-file-data-format" value="FormatFloat"/></div><div> <Option name="native-file-header-forma<wbr>t" value="WAVE"/></div><div> <Option name="auto-play" value="no"/></div></div>
</blockquote></div><br></div>