Re: [Libreoffice] [PATCH] Replace (Byte)String with O(U)String

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).