On 01/17/2012 09:45 PM, Chr. Rossmanith wrote:
this is a modified version of the previous patch which takes into account improvements suggested by Caolan and Stephan. Please don't push yet because the caller of RecodeString() has to be touched as well. Does that modification have to be contained in the same patch or do I just have to make sure that the two commits are pushed together? A second review is very welcome.
It is best to do it as a single patch (doing it in two steps would produce problems for git bisect, for example).
The patch looks good now (only nit to pick is to consider moving from an in--out parameter to an in-only parameter and rtl::OUString return type; out parameters, esp. if they "hide" behind a reference type, can be confusing -- think about somebody trying to grok where a given variable aStr can change its value, and overlooking something innocuous-looking like
mpFontEntry->mpConversion->RecodeString(aStr); where aStr = mpFontEntry->mpConversion->RecodeString(aStr); would be so much clearer). _______________________________________________ LibreOffice mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/libreoffice
