On Feb 20, 2014, at 2:34 PM, Johan Engelen wrote:
On 20-2-2014 20:25, Martin Owens wrote:
On Thu, 2014-02-20 at 18:58 +0100, Johan Engelen wrote:
- Please don't add functions that use C-strings unnecessarily. Simply rewrite parseDataUri to parseDataUri(const std::string
&uri),
I have a question about this revert. The logic is being copied from image.cpp (where it uses c-strings) and I figured calling std::string( uri ) on a massive embedded image uri would be bad.
So, should I rewrite uri.cpp to only use std::string or accept the string copy that might result in lots of member use?
I don't fully understand your question, but eventually, pretty much everything of Inkscape should use std::string. So it's good not to add any C-string stuff, to make the refactoring easier. I agree that copying a massive data structure, only to discard it after does not make sense. I'm sorry, I didn't think enough about the function's use cases. Perhaps in this case, indeed it is better to use char*, if that is how the uri was stored in the first place. So you'd have to look at who will call parseDataUri and if they have the uri in a std::string or char*. (if so, you could add a brief comment of why you think char* is preferred above std::string)
In general we should look to using either std::string or a std::vector<...> of some proper type. (The latter comes into play where code uses char * as "random array of bytes" instead of "null-terminated 8-bit string"). For using and passing a string we can use 'std::string const &inputData' to avoid duplication (passing a reference to a const string so that the string is not duplicated).
In this specific case we might benefit from something more. A data URI might be representing some binary blob that we could store in non-string form. Accessing a std::string as a c-string is fairly simple, but not vice-versa. I'll take a look to see what might be appropriate for the code in question. Also there are quite a few smart pointers in both Boost and C++ we can look into (but carefully).
Oh, and yes, creating a std::string from a large DataUI can be problematic.