On 20-2-2014 20:12, Martin Owens wrote:
On Thu, 2014-02-20 at 18:58 +0100, Johan Engelen wrote:
Hi Martin, I'm sorry, I will revert commit 13045.
Why be sorry, everything I've written recently has been reverted. While I might be annoyed, it's not with your code review, just with myself.
- It contains a memleak
realpath allocates new memory if the second argument is NULL. In the added code, this memory is never freed.
Realpath is a serious concern for me. I spent two hours researching path normalisation in c++ only to discover that it's impossible because glib is insufficient. :-/
If it doesn't start behaving I will fork inkscape and rewrite it a proper language like python. ;-)
If you happen to know about a C++ path normalisation function, do let me know as it will save me an awful lot of time.
I only briefly glanced over the link in my earlier message. But it seems quite elaborate indeed. Perhaps you can use the Boost function suggested there. (we already have Boost as a dependency) If you still want to use realpath (I think it's called fullpath on Windows), at least make sure you clean up memory afterwards.
About memory clean-up: Your " preformed = ""; // g_free? " , indeed should not use g_free. What you are doing is having a local (!) pointer point to something else. You should not free the memory pointed to by the pointer passed to the function. So what you are doing is correct: you have the local (!) argument pointer point to another string instead. No memleak there.
cheers, Johan