GNOME Bugzilla – Bug 765044
Numeric TreeView cells throw exceptions
Last modified: 2017-10-23 15:43:18 UTC
std::invalid_argument exception is thrown in _auto_store_on_cellrenderer_text_edited_numerical when bad numeric value is entered (std::stod throws them). This makes TreeView practically unusable because these exceptions crash the whole application. My suggestion would either be to write the offending part like this: //Convert the text to a number, using the same logic used by GtkCellRendererText when it stores numbers. ColumnType new_value = ColumnType(); try { new_value = static_cast<ColumnType>(std::stod(new_text)); } catch (std::invalid_argument&) { // Ignore the exception; default value will be set } //Store the user's new text in the model: Gtk::TreeRow row = *iter; row.set_value(model_column, new_value); or to use strtod and friends. For instance, template<typename T> struct string_converter { static T convert(const Glib::ustring& str) { // use strtod by default } } ... add specializations for strtof, strtol, strtoul, etc. At any rate, conversion to numeric value MUST NOT crash the application.
Thanks. Indeed. I caused this regression here: https://git.gnome.org/browse/gtkmm/commit/gtk/src/treeview.hg?id=fcc0f358d17264f8956567a33471a6b52c2fc0d1 Unlike the strtod() C function, http://www.cplusplus.com/reference/cstdlib/strtod/ std::stod() does throw an exception: http://www.cplusplus.com/reference/string/stod/ For now I have put a try/catch around it, as you suggest: https://git.gnome.org/browse/gtkmm/commit/?id=f0c52c2d515f955c8b186e1394933d10efef65b9 (and fixed my whitespace in the next commit. Sorry.) I like your idea of having specializations for the part that uses stod. Would you like to submit a git patch for that?
(In reply to Murray Cumming from comment #1) > Thanks. Indeed. I caused this regression here: > https://git.gnome.org/browse/gtkmm/commit/gtk/src/treeview. > hg?id=fcc0f358d17264f8956567a33471a6b52c2fc0d1 > > Unlike the strtod() C function, > http://www.cplusplus.com/reference/cstdlib/strtod/ > std::stod() does throw an exception: > http://www.cplusplus.com/reference/string/stod/ > > For now I have put a try/catch around it, as you suggest: > https://git.gnome.org/browse/gtkmm/commit/ > ?id=f0c52c2d515f955c8b186e1394933d10efef65b9 > (and fixed my whitespace in the next commit. Sorry.) > > I like your idea of having specializations for the part that uses stod. > Would you like to submit a git patch for that? Sure. I can make the patch over the weekend.
Created attachment 326076 [details] [review] More efficient numeric conversions for TreeView
Hm, now that I look at my patch I see that I do not handle int. The easiest way to handle this would probably be like this GTKMM_TREEVIEW_NUMERIC_CAST_SPECIALIZATION(int, (int)strtol, nullptr, 0) which also avoid warnings that long is casted to int.
_auto_store_on_cellrenderer_text_edited_numerical() contains this comment: //std::istringstream astream(new_text); //Put it in a stream. //ColumnType new_value = ColumnType(); //new_value << astream; //Get it out of the stream as the numerical type. The last line is wrong. Should be //astream >> new_value; //Get it out of the stream as the numerical type. Isn't it a good idea to use istringstream's input operator? It is overloaded for all standard numeric types. I don't know if it's more efficient, but it's nice to make use of an overloaded operator in standard C++ instead of adding our own equivalent overloading.
Sure. I didn't know there was any difference.
Created attachment 332006 [details] [review] patch: TreeView: Use std::istringstream to convert a text to a number Here's an alternative to Justinas' patch. Much less code, when an std::istringstream is used. There are minor differences, compared to using std::stod(): std::stod() converts a string to a double, which is then cast to ColumnType. Then e.g. "2e3" will be converted to 2000, even if ColumnType is int. istringstream will convert "2e3" to 2. istringstream handles out-of-range values reasonably. They are replaced by the minimum or maximum value of ColumnType. Casting a very large double value to an int might not behave reasonably. I deleted to comment "using the same logic used by GtkCellRendererText when it stores numbers" because I can't see that GtkCellRendererText stores numbers. Does it?
Created attachment 332024 [details] [review] patch: TreeView: Use std::istringstream to convert a text to a number It's not quite as easy as I thought. std::istringstream::operator>>() does not behave like std::strtod() and std::stod() when it comes to locales and decimal delimiters (point or comma). C functions don't choose locale in the same way as C++ functions. In this respect std::strtod() and std::stod() behave like C functions, while the members of C++ streams behave like C++ functions. With the patch in comment 7, on my Ubuntu system with LANG=sv_SE.UTF-8, std::strtod() and std::stod() require a decimal comma in floating-point numbers with decimals, but std::istringstream::operator>>() requires a decimal point. My new patch fixes that. std::istringstream::operator>>() also requires a decimal comma. Glib::init() and Gtk::init() don't handle this complication well. I think my patch in the glibmm bug 661588 comment 6 would make life easier. But I suppose it breaks API.
A very lightweight patch. :-} That's exactly why I implemented converter specializations the way I did in my suggestion. Do you *really* want to create an stringstream object and to imbue locale for _every numeric input conversion_? No, thank you. The current try/catch is better.
Comment on attachment 332024 [details] [review] patch: TreeView: Use std::istringstream to convert a text to a number I agree that this is a bad patch. My intention was to make the code as short as it is at present, but specialized for each numeric type. When I added imbue(), that intention totally failed. The underlying problem here is that the C and C++ global locales differ after a call to Gtk::Application::create() or Gtk::Main::Main(). If that could be fixed, my shorter patch in comment 7 would work. That patch is at least worth considering. See also bug 661588 comment 8.
Created attachment 342377 [details] [review] patch: TreeView: Use std::istringstream to convert a text to a number (gtkmm4) Bug 661588 has been fixed in glibmm-2.52 and gtkmm-4.0. Glib::init() and Gtk::Application::create() set the C and C++ global locale to the user's preferred locale. This means std::istringstream's input operator can be used without creating a new locale each time it's used. Also with this version of my patch, a std::istringstream object is created for each numeric input conversion. I don't know how costly this is.
I have pushed a patch that uses the std::strto*() functions. https://git.gnome.org/browse/gtkmm/commit/?id=b90f88423b72c99da8e971e2bd23ff64c39b21aa The std::sto*() functions are more C++-like. They take a const std::string& parameter and throw exceptions on errors, but the exceptions are a nuisance in this case.
I agree with the patch, even though there was a reason why I didn't use explicit function template specializations. Anyway, this is a small detail. More pressing things are these: 1. More C++-esque way to check for the limits is std::numeric_limits<T>::min/max. That is if you strive for C++ elegance. Absolute deal breaker is this: 2. Conversion functions MUST be inline, unless you want to potentially violate the ODR rule. Don't forget that this header might be included in various compilation units and those functions would get compiled into each of them. Which would fail at link time.
I know that there are risks with function template specializations. This issue was discussed in the libsigc++ bug 724496. Two known risks: 1. ADL (argument-dependent lookup) can make the compiler search for overloaded functions in unexpected places. 2. If there are two or more overloaded primary function templates, the compiler might not select a specialization that exactly matches the argument types, even if there is one. IMO these risks are negligible in this case. They can be eliminated by changing the function call to Gtk::TreeView_Private:: _convert_from_ustring_to_numeric_type<ColumnType>(new_text), assuming that no user adds anything to namespace Gtk::TreeView_Private. Comments to your more pressing issues: 1. You're right. I can change the checks. Also, std::min() and std::max() are usually nicer than the ?: operator. Unfortunately std::min/max require some extra static_casts in this case, because both arguments must have exactly the same type. Then it's not so nice any more. That's why I chose ?:. 2. I don't understand. AFAIK "conversion function" is not a special type of function that the compiler treats differently from other functions. Do you mean that the primary function template must be declared with the 'inline' keyword because it's defined in the header file? Or must the specializations be defined (not just declared) in the header file and marked 'inline'?
I was saying that _convert_from_ustring_to_numeric_type function needs to be inline. It is this function that I called a "conversion function". I did this for brevity, since the name of that function is rather long. All definitions of that function must be marked inline, including explicit specializations. This must be done because otherwise those functions will be compiled in all compilation units (cpp files, if you want) that include the treeview header, and this will cause failures at link time. This should get you started on the topic: https://en.wikipedia.org/wiki/One_Definition_Rule As for the bounds checking, I simply don't care what and how you do. It's up to you. I just pointed the way that may help you refactor the code. Consider this: why you need several implementations of bounds checking, if you can write this ONCE as template<typename Result, typename T> inline Result _clamp_to_numeric_type(T value) { typedef std::numeric_limits<Result> limit_t; return static_cast<Result>( std::min(std::max(value, static_cast<T>(limit_t::min())), static_cast<T>(limit_t::max())) ); } and then simply return like _clamp_to_numeric_type<int>(std::strtol(text.c_str(), nullptr, 0)); Nevertheless, I am not a gtkmm developer, I am a user, so I don't really care if you refactor your code behind the scenes, or not. It's strictly up to you. However, I do care a great deal that my code links. Please fix.
Don't be so upset. I'm just a gullible programmer who have believed in what Bjarne Stroustrup writes in "The C++ Programming Language", 4th edition, section 23.7 "Source Code Organization". There he shows an example with a template function definition in a header file template<typename T> void out(const T& t) { std::err << t; } and that header file included in user1.cpp and user2.cpp. Cited from section 23.7: "That is, the definition of out() and all declarations on which it depends are #included in several different compilation units. It is up to the compiler to generate code when needed (only) and to optimize the process of reading redundant definitions. This strategy treats template functions the same way as inline functions." ... "As ever, non-inline, non-template functions and static members must have a unique definition in some compilation unit." If you don't draw the same conclusion from this text as I do, I can add 'inline' to the primary definition of the template function. I still don't understand why the specializations of _convert_from_ustring_to_numeric_type() must be inline. They are defined in treeview.ccg, which becomes treeview.cc when gtkmm is built. The specializations are only declared (not defined) in treeview.hg. How can there be more than one definition, even if they are called in more than one compilation unit? In gtkmm there are other template functions which are not explicitly marked 'inline', such as template<class T> T* manage(T* obj) { obj->set_manage(); return obj; } That code has not been changed for more than 13 years.
Of course Bjarne is correct. I must admit that it is I who is wrong here. I imagined that all those explicit specializations of _convert_from_ustring_to_numeric_type will be in the header file. And explicitly specialized template functions are not template functions anymore -- they are just ordinary functions, so the above description from section 23.7 would not apply to them. Now that the confusion is cleared up, I am pretty happy with the patch.