Re: [Libreoffice] [PATCH] BUG 36594

Hi Jenei,

On Sunday, 2011-08-21 14:59:26 +0200, Jenei Gábor wrote:

> +    ::rtl::OUStringBuffer sTemp;

Construct the buffer with a sufficient capacity beforehand so no memory
allocation needs to be done in between:

       ::rtl::OUStringBuffer sTemp( nQueryLen);

> [...]
> +    return (::rtl::OUString)sTemp;

Make that instead

       return sTemp.makeStringAndClear();

With your C-style cast (please don't use those anyhow, this is C++)
a temporary string instance is created and the buffer is copied, which
it isn't when using makeStringAndClear().


> +std::vector< ::rtl::OUStringBuffer > getComment(const ::rtl::OUString& sQuery){

Why return vector<OUStringBuffer> ? vector<OUString> seems to be more
straight forward.

> +    const sal_Unicode* sCopy=sQuery.getStr();
> +    int nQueryLen=sQuery.getLength();
> +    bool bIsText1=false;
> +    bool bIsText2=false;
> +    bool bComment=false;
> +    std::vector< ::rtl::OUStringBuffer > sRet;
> +    ::rtl::OUStringBuffer sTemp;
> +    for(int i=0;i<nQueryLen;i++){
> +        if(sCopy[i]=='\"' && !bIsText2 && !bComment) bIsText1=!bIsText1;
> +        if(sCopy[i]=='\'' && !bIsText1 && !bComment) bIsText2=!bIsText2;
> +        if(sCopy[i]=='\n' || i==nQueryLen-1){
> +            if(bComment) bComment=false;
> +            sTemp.append((sal_Unicode)'\n');

> +            sRet.push_back(sTemp);
> +            sTemp=::rtl::OUString();

Provided that vector<OUString> is returned, change that to

               sRet.push_back( sTemp.makeStringAndClear());

then there's also no need to assign sTemp an empty OUString anymore.

> +        }
> +        if(!bIsText1 && !bIsText2 && (i+1)<nQueryLen && sCopy[i]=='-' && sCopy[i+1]=='-') 
> bComment=true;
> +        if(!bIsText1 && !bIsText2 && (i+1)<nQueryLen && sCopy[i]=='/' && sCopy[i+1]=='/') 
> bComment=true;
> +        if(bComment) sTemp.append(&sCopy[i],1);
> +    }
> +    sTemp=::rtl::OUString();

That one is superfluous, sTemp as a stack local variable will be
destructed upon return anyway.

> +    return sRet;
> +}


> +//------------------------------------------------------------------------------
> +::rtl::OUString ConcatComment(const ::rtl::OUString& sQuery,std::vector< ::rtl::OUStringBuffer > 
> sComments){

Pass sComments as const reference so it doesn't need to be copied when
ConcatComment() is called. And use vector<OUString> instead, as above.

> +    ::rtl::OUStringBuffer sRet;
> +    int nIndLF=0;
> +    int nIndBeg=0;
> +    for(unsigned int i=0;i<sComments.size();i++){
> +        nIndBeg=nIndLF;
> +        if(sQuery.indexOf('\n')==-1){
> +            nIndLF=sQuery.getLength();

This conditional gets executed for each element of sComments, but
clearly sQuery can't change in between, so doing this over and over
again is unnecessary.

> +        }
> +        else{
> +            nIndLF=sQuery.indexOf('\n',nIndLF);
> +        }
> +        sRet.append(sQuery.copy(nIndBeg,nIndLF-nIndBeg));

What if sQuery doesn't contain as many LFs as there are elements in
sComments? nIndLF will be -1 and in the next loop run nIndBeg will also
be -1.

The append(copy()) construct could be optimized as the copy creates
a temporary OUString instance that then is appended to sRet. More
effective would be something like

           sRet.append( sQuery.getStr() + nIndBeg, nIndLF - nIndBeg));

> +        sRet.append(sComments[i]);
> +    }
> +    return (::rtl::OUString)sRet;

Same here, use

       return sRet.makeStringAndClear();



Btw,

> +            sTranslatedStmt=ConcatComment(sTranslatedStmt,sComments);

blanks increase legibility, maybe it's just me but I'd much prefer a

               sTranslatedStmt = ConcatComment( sTranslatedStmt, sComments);

style instead.

Thanks
  Eike

-- 
 PGP/OpenPGP/GnuPG encrypted mail preferred in all private communication.
 Key ID: 0x293C05FD - 997A 4C60 CE41 0149 0DB3  9E96 2F1A D073 293C 05FD

Attachment: signature.asc
Description: Digital signature