On Jul 27, 2010, at 9:52 PM, bulia byak wrote:
Why do you think that this is where this has to be fixed, instead of switching the filters dialog to static strings? Fill&stroke uses static strings just fine.
I don't see any necessity for dynamic generation of keys, and doing a string dup on each undo event sounds unnecessarily wasteful.
In any case, this is a nontrivial change in a very sensitive area, so I would prefer to err on the side of caution and not apply it to 0.48, given that the problem it fixes is relatively cosmetic.
I think Krzysztof might have miscommunicated some of what he was seeing. From his email I also got the same impression, but once I looked at the actual diff it was clear there were problems in the implementation of sp_document_maybe_done().
It's not such much that dynamic generation of keys was the main issue, but that SPDocument was holding onto a temporary pointer retuned from Glib::ustring, which in turn is just std::string::c_str().
So the problem comes up that later on when we go to compare things to the next string, the pointer is invalid and could be pointing anywhere. Holding stale pointers can easily cause crashes. Some OS's are also more susceptible to these.. Erring on the side of caution would probably say not to hold stale pointers.
If we're really concerned about performance, we should take a look at passing in a reference to a Glib::ustring. The C++ classes usually have been optimized to share buffers, etc. Of course we could probably search through all the calls to sp_document_maybe_done() and ensure they only use static persistent strings, but we still should look for ways to keep the code safe.