Re: [Libreoffice] [PATCH] Cleaned up in dpsave.hxx/.cxx

Hi Soeren,

On Mon, 2011-01-31 at 22:33 +0100, Soeren Moeller wrote:
> Hi
>
> Here five patches cleaning up in dpsave.hxx/.cxx in sc
> 0001: replacing tools/solar.h data types by sal data types
> 0002: cleaning up spacings
> 0003-0005: replacing String with OUString in the three classes, and
> changing necessary call sites
> the patches compile fine and calc seems to work as expected.

Good.  I applied all of them, but I have some feedback.

Patch 0002 basically does change the coding styles.  Most of the changes
looked ok and I applied the whole patch, but some parts were a bit too
aggressive.  For instance, in some class declaration the class data
member names were aligned as well as their data types (like this)

SomeData   maData;
OtherData  maOther;
MyFoo      maFoo;
..

and you've basically flattened them to

SomeData maData;
OtherData maOther;
MyFoo maFoo;

While this didn't bother me (and applied it), I'm sort of neutral on
this, and other people may have different preferences.

Also, I had to make some follow-up changes to some of your patches.

http://cgit.freedesktop.org/libreoffice/calc/commit/?id=750e245c50ac0bac25305ed579bd3cc80963e916

The gist of it is that, when the original string is of OUString type,
and the code creates a String instance from OUString instance, instead
of creating yet another OUString instance from the 2nd String instance,
let's eliminate the 2nd String instance and use the original OUString
one directly.  For example, instead of doing

void someFunc(const OUString& rName)
{
    String aCopy = rName;
    ....
    if (pDim->GetName() == OUString(aCopy))
    {
        ....

it's better to 

void someFunc(const OUString& rName)
{
    if (pDim->GetName() == rName)
    {
        ....

and cleaner this way.

Also this follow-up commit
http://cgit.freedesktop.org/libreoffice/calc/commit/?id=000bc293630164213aa5051d71c04febc0bf038c

uses rtl::OUStringBuffer instead of rtl::OUString when you need to
append to an existing string value many times.  With plain OUString,
each time you append a new string segment, it creates a new string
instance instead of appending to the original string.  OUStringBuffer
doesn't do that, as it appends to the original string array.  So,
whenever you need to append to the original string instance,
OUStringBuffer should be the preferred choice.

> dpsave.hxx/.cxx still depend on tools/string as it is used in DBG_...
> macros (supposedly from tools/debug), are those still used/meaningful?

I don't use them, and I don't mind them gone.  Feel free to remove
them. ;-)

Kohei

-- 
Kohei Yoshida, LibreOffice hacker, Calc
<kyoshida@novell.com>