On 24-3-2014 21:57, Krzysztof Kosiński wrote:

2014-03-24 21:16 GMT+01:00 Johan Engelen <jbc.engelen@...2592...>:

...

Hi all, To easier use repr->setAttribute, I want to add two extra (overloaded) functions

  virtual void setAttribute(gchar const *key, gchar const *value,

bool is_interactive=false)=0; virtual void setAttribute(gchar const *key, Glib::ustring const &value, bool is_interactive=false)=0; virtual void setAttribute(Glib::ustring const &key, Glib::ustring const &value, bool is_interactive=false)=0;

Should I use std::string or Glib::ustring for this?

The XML tree currently uses the GC allocator, so to avoid memory leaks you cannot use any class that uses the standard allocators within it. You would have to use std::string with a nonstandard allocator.

If this is only about adding an interface without any internal changes, then I think Glib::ustring is the better choice, since the XML data inside is in UTF-8.

Indeed for now, convenience interface functions. Thanks, I agree with Glib::ustring.

Can you have a look at rev. 13203 and make amendments if necessary?

Thanks, Johan

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.

The main gotcha would be if we are passing NULL into any of those calls as an expected use pattern.

However... there is enough there that I'll give it another look. I think a few things can be tuned up.

On Mar 24, 2014, at 5:09 PM, Johan Engelen wrote:

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.

Aha! So what was the measured real-world difference in performance of a non-debug build for normal operations? :-)

In general, if we don't have a measurable difference, it's usually best to avoid micro-optimizations and instead just stick with code safety. in this case, a Glib::ustring const &key declaration means NULL can't sneak in while we are not looking, etc. In some similar code changes in the past, we've actually had bugs from raw pointer use, especially when a pointer the function assumed was constant turned out to be a temporary/local and deleted by the time it was used again.