On 25-3-2014 1:03, Jon Cruz wrote:
On Mar 24, 2014, at 3:07 PM, Krzysztof Kosiński wrote:
It looks OK, no amendments necessary.
I'm not 100% sure about the "hybrid" variant which takes the key as char const * and the value as Glib::ustring, but this is a common occurrence in the codebase, so I guess this optimization is justified.
Johan, can you take a look at that? Normally I'd expect an implicit conversion to come into play and the compiler to wrap char const* params in a Glib::ustring instance. That definitely is the usual behavior with std::string.
I did that as an optimization step. Very often we use setAttribute("something", some_variable); I thought it silly to have to compiler create a temporary Glib::ustring for that.
The main gotcha would be if we are passing NULL into any of those calls as an expected use pattern.
I believe we are using that functionality in plenty of places for the value parameter. (passing NULL for value means something like "remove the attribute completely").
The check for empty key-value and replacing it with NULL is a mistake from my side I think.
cheers, Johan