TempGString to simplify common pattern in code..?
Hi guys!
One thing I noticed while browsing around Inkscape source, was the pattern "allocate temp gtk string, call some api, deallocate temp string".
I think it makes sense to make a small "TempGString" class that captures this (RAII) pattern, and deallocates the string on going out of scope. This would have some advantages compared to current approach:
1) string would be freed regardless of how the code exits the block (exceptions e.g.) 2) 2 lines instead of 3 3) easier on the eyes
Sample before-after code snippet:
Before: if (!qname_prefix(code).id()) { gchar *svg_name = g_strconcat(prefix, ":", g_quark_to_string(code), NULL); repr->setCodeUnsafe(g_quark_from_string(svg_name)); g_free(svg_name); }
After: if (!qname_prefix(code).id()) { TempGString svg_name(g_strconcat(prefix, ":", g_quark_to_string(code), NULL)); repr->setCodeUnsafe(g_quark_from_string(svg_name)); }
Syntax highlighted version available here: http://pastebin.com/t7p0TVfj
The constructor takes the freshly allocated string, and the destructor calls g_free on it. There is an operator overload so that contexts that expect ordinary pointers to gchar* gets what it want. This automagic casting is a point I'm a bit ambivalent about myself; I wouldn't mind having an accessor function "get_gchar_ptr()" or similar just to be super explicit, thoughts on that?
I don't think this would break anything, however it might be that I'm ignorant to some weird behaviour of ghar* or glib/gtk?
On a convention/code style notion, I looked through the code for RAII (worst abbreviation for something so clear/simple semantically) and noticed it's used in a couple other places, however I've still been wary of introducing this refactoring to the code base as I don't know if you guys would like it.
The pattern occurs on at least 140 places in the code base.
Cheers,
/Olof
Does anyone want to voice an opinion on this one or should I just go ahead and do it? What about using implicit casting, you OK with that?
I think reviewing it should be fairly straight-forward as it's almost a regex search-replace operation (just almost heh).
BTW I noticed there is a paste option on the web site too, so here it is:
https://inkscape.org/en/paste/9559/
Mvh
/Olof ----------------- Är du systemutvecklare? Spana in https://cilamp.se
On 3 May 2016 at 22:29, Olof Bjarnason <olof.bjarnason@...400...> wrote:
Hi guys!
One thing I noticed while browsing around Inkscape source, was the pattern "allocate temp gtk string, call some api, deallocate temp string".
I think it makes sense to make a small "TempGString" class that captures this (RAII) pattern, and deallocates the string on going out of scope. This would have some advantages compared to current approach:
- string would be freed regardless of how the code exits the block
(exceptions e.g.) 2) 2 lines instead of 3 3) easier on the eyes
Sample before-after code snippet:
Before: if (!qname_prefix(code).id()) { gchar *svg_name = g_strconcat(prefix, ":", g_quark_to_string(code), NULL); repr->setCodeUnsafe(g_quark_from_string(svg_name)); g_free(svg_name); }
After: if (!qname_prefix(code).id()) { TempGString svg_name(g_strconcat(prefix, ":", g_quark_to_string(code), NULL)); repr->setCodeUnsafe(g_quark_from_string(svg_name)); }
Syntax highlighted version available here: http://pastebin.com/t7p0TVfj
The constructor takes the freshly allocated string, and the destructor calls g_free on it. There is an operator overload so that contexts that expect ordinary pointers to gchar* gets what it want. This automagic casting is a point I'm a bit ambivalent about myself; I wouldn't mind having an accessor function "get_gchar_ptr()" or similar just to be super explicit, thoughts on that?
I don't think this would break anything, however it might be that I'm ignorant to some weird behaviour of ghar* or glib/gtk?
On a convention/code style notion, I looked through the code for RAII (worst abbreviation for something so clear/simple semantically) and noticed it's used in a couple other places, however I've still been wary of introducing this refactoring to the code base as I don't know if you guys would like it.
The pattern occurs on at least 140 places in the code base.
Cheers,
/Olof
This sounds like something that should already be covered by gtkmm or even std::string. Usually it's better to look first to existing solutions before inventing our own wheel.
IIRC Glib::ustring should cover the cases where we have content that is guaranteed to be UTF-8. (Keep in mind that gtk+ apps have to keep track of three different encodings and not get them mixed up.
For strings that are potentially different encodings we can use std::string.
For content that looked like a string but is actually a byte sequence, we can use std::vector<char> or std::vector<uint8_t>
On Fri, May 6, 2016, at 05:38 AM, Olof Bjarnason wrote:
Does anyone want to voice an opinion on this one or should I just go ahead and do it? What about using implicit casting, you OK with that?
I think reviewing it should be fairly straight-forward as it's almost a regex search-replace operation (just almost heh).
BTW I noticed there is a paste option on the web site too, so here it is:
-- Jon A. Cruz jon@...18...
Yeah, of course if there already exists a solution / pattern then that would be preferrable...
(I have only looked at cases for deallocating gchar* using g_free, I do not know what encoding is used inside the strings, and a TempGString object would be agnostic to the encoding anyway.)
Anyone know if there is a RAII type for string handling in GLib? My google fu fails me on this atm.
Mvh
/Olof ----------------- Är du systemutvecklare? Spana in https://cilamp.se
On 6 May 2016 at 15:40, Jon A. Cruz <jon@...18...> wrote:
This sounds like something that should already be covered by gtkmm or even std::string. Usually it's better to look first to existing solutions before inventing our own wheel.
IIRC Glib::ustring should cover the cases where we have content that is guaranteed to be UTF-8. (Keep in mind that gtk+ apps have to keep track of three different encodings and not get them mixed up.
For strings that are potentially different encodings we can use std::string.
For content that looked like a string but is actually a byte sequence, we can use std::vector<char> or std::vector<uint8_t>
On Fri, May 6, 2016, at 05:38 AM, Olof Bjarnason wrote:
Does anyone want to voice an opinion on this one or should I just go ahead and do it? What about using implicit casting, you OK with that?
I think reviewing it should be fairly straight-forward as it's almost a regex search-replace operation (just almost heh).
BTW I noticed there is a paste option on the web site too, so here it is:
https://inkscape.org/en/paste/9559/
-- Jon A. Cruz jon@...18...
Find and fix application performance issues faster with Applications Manager Applications Manager provides deep performance insights into multiple tiers of your business applications. It resolves application problems quickly and reduces your MTTR. Get your free trial! https://ad.doubleclick.net/ddm/clk/302982198;130105516;z _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel
OK, I read up a little on GLib and GTK+.
Both are (pure) C libraries, so I fail to see how they could provide a RAII idiom? Last time I checked, C didn't have constructors/destructors...? Or maybe there is some pre-processor magic to make it happen...?
std::string is nice (I would prefer it in fact) but it wouldn't call g_free automatically when the gchar* variable goes out of scope.
Mvh
/Olof ----------------- Är du systemutvecklare? Spana in https://cilamp.se
On 6 May 2016 at 15:51, Olof Bjarnason <olof.bjarnason@...400...> wrote:
Yeah, of course if there already exists a solution / pattern then that would be preferrable...
(I have only looked at cases for deallocating gchar* using g_free, I do not know what encoding is used inside the strings, and a TempGString object would be agnostic to the encoding anyway.)
Anyone know if there is a RAII type for string handling in GLib? My google fu fails me on this atm.
Mvh
/Olof
Är du systemutvecklare? Spana in https://cilamp.se
On 6 May 2016 at 15:40, Jon A. Cruz <jon@...18...> wrote:
This sounds like something that should already be covered by gtkmm or even std::string. Usually it's better to look first to existing solutions before inventing our own wheel.
IIRC Glib::ustring should cover the cases where we have content that is guaranteed to be UTF-8. (Keep in mind that gtk+ apps have to keep track of three different encodings and not get them mixed up.
For strings that are potentially different encodings we can use std::string.
For content that looked like a string but is actually a byte sequence, we can use std::vector<char> or std::vector<uint8_t>
On Fri, May 6, 2016, at 05:38 AM, Olof Bjarnason wrote:
Does anyone want to voice an opinion on this one or should I just go ahead and do it? What about using implicit casting, you OK with that?
I think reviewing it should be fairly straight-forward as it's almost a regex search-replace operation (just almost heh).
BTW I noticed there is a paste option on the web site too, so here it is:
https://inkscape.org/en/paste/9559/
-- Jon A. Cruz jon@...18...
Find and fix application performance issues faster with Applications Manager Applications Manager provides deep performance insights into multiple tiers of your business applications. It resolves application problems quickly and reduces your MTTR. Get your free trial! https://ad.doubleclick.net/ddm/clk/302982198;130105516;z _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel
participants (2)
-
Jon A. Cruz -
Olof Bjarnason