On Mar 4, 2014, at 12:54 PM, mathog wrote:
On 03-Mar-2014 13:47, Martin Owens wrote:
I'm going to rip out c-strings from uri handling. It's dangerous code and needs to be dealt with.
A c string is only "dangerous" if the programmer mistakenly thinks it works the same as std::string. Conversely, all changes to working code are dangerous since a modification introduces the risk of creating a bug and breaking the program.
Actually, there are many different factors that make C-strings "dangerous" and bad for code. Dangling pointers, buffer overruns, size mismatches, memory allocation, etc. all are problematic and have presented in errors in Inkscape.
Yes, change itself is risky, but change to improve maintainability of the code has reduced problems in the long run.
For the URIs it doesn't matter much what the internal storage method is, so go ahead and change that - in your branch. For bitmap images the underlying libraries, like libPNG, are not going to play nicely with std::string, and it seems a little silly to waste time and memory converting back and forth all the time, since either way in the end Inkscape just carries around a big block of memory.
Actually... libPNG and such play just fine with std::string. However they also work well with std::vector<guchar> which is often much more appropriate. With C-strings you might have a c-string, or might have a random byte buffer.
You are right about converting too much, however. With URIs and images we have some bad overhead now due to conversions and inefficient storage (data URI's expand the data significantly as we maintain binary versions and base-64 expanded string versions). So Inkscape is not just carrying around a big block of memory but a big block of memory *plus* a second 33% larger block of memory at the same time.
Similarly, a lot of glib uses gchar * (or guchar *) arguments, and some of those functions return values in those formats. It is far from clear to me that requiring everything that talks to glib to convert to/from std::string will reduce the number of bugs in Inkscape.
We do need to clear up a *lot* of those. And over time doing such cleanup has been seen to address Inkscape bugs.
To feed something to glib we just need to feed them 'thing.c_str()' or '&thing[0]'. For many other things there are C++ lib calls that either wrap or replace those and that we should use instead.
Oh, and to be clear... one of the *big* safety things we can do is switch to only use Glib::ustring/std::string for actual strings, and std::vector<guchar>, etc. for non-string binary data. C++ added such distinctions since lack of such was a big problem and source of bugs in C code.