Re: [Libreoffice] [PATCH] BUG 36594

Hello Eike,

Let me to have some questions about your review:

2011. 08. 22. 21:47 keltezéssel, Eike Rathke írta:
> 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.
Why isn't it needed? sTemp is not local in the loop, so it would contain 
the old line still, so for me it seems that it would do bad push_back. 
Maybe you wanted to tell to declare sTemp inside the loop so it'll be 
local in the loop?
> > +        }
> > +        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.
yes,thank you,this is a needless line in the end.
> > +    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.
Yes,this is also ok, I've already modified it, passing classes by value 
may need long time and memory.
> > +    ::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.
So,should I do a break when the condition is true(anyway if there is no 
linefeed in the query it implies that the length of sComments is 1, you 
can see this in getComments function, so the if will be evaluated only once.
> > +        }
> > +        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.
Well, I have just told a few lines ago, this is theorically impossible, 
as we used getComment function, which even for lines without comment 
pushes a linefeed just alone,in order to know the correct order of 
lines. So, I guess your problem only may occur if someone uses this 
function without getComment, this might be buggy, yeah.
> 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
>
>
>
> _______________________________________________
> LibreOffice mailing list
> LibreOffice@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/libreoffice
Sorry for this long mail,but I thought it's better to see the whole 
former code, I hope you saw all my questions.

Thank you for your help:

Gabor