[Libreoffice] [PATCH] BUG 36594

[Eike Rathke]

Hi Jenei,

On Monday, 2011-08-29 19:10:32 +0200, Jenei Gábor wrote:

> Let me to have some questions about your review:

Sure.

> >>+    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?

No, the makeStringAndClear() also sets the buffer to length 0, hence the
Clear in the name ;-)


> >>+    ::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.

I'd do the check once before the loop and execute the loop only if the
result is >=0

> >>+        }
> >>+        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.

Yes, that's what I meant, as long as both functions are used together in
the way they were intended it may be ok, but you don't know if someone
at some point wouldn't use them separately for whatever reason, so
setComment() should be able to cope with unexpected data, at least not
have indices point outside the query string and break the loop if nIndLF<0

With the check before the loop that could be

    const sal_Unicode* pBeg = sQuery.getStr();
    const sal_Int32 nLen = sQuery.getLength();
    sal_Int32 nIndBeg = 0;
    sal_Int32 nIndLF = sQuery.indexOf('\n');
    for (size_t i=0; nIndLF >= 0 && i < sComments.size(); ++i)
    {
        sRet.append( pBeg + nIndBeg, nIndLF - nIndBeg));
        sRet.append( sComments[i]);
        nIndBeg = nIndLF + 1;
        nIndLF = (nIndBeg < nLen ? sQuery.indexOf( '\n', nIndBeg) : -1);
    }
    if (nIndBeg < nLen)
        sRet.append( pBeg + nIndBeg, nLen - nIndBeg));

Note the changes I made:

* obtain pBeg once
* obtain nLen once
* sal_Int32 instead of int
* size_t instead of unsigned int
* pre-increment ++i instead of i++ post-increment (doesn't matter with
  plain types but with iterators, so it's good habit)
* the append() without constructing a temporary OUString
* nIndBeg = nIndLF + 1;  instead of  nIndBeg = nIndLF;
  otherwise it would loop forever, and we don't want the LF included.
* indexOf() only if start point is within string
* if (nIndBeg < sQuery.getLength()) checks whether anything still needs
  to be appended that wasn't yet. This should not happen in your use
  case, but.. at least the query isn't truncated if so.
* blanks for readibility

All written from scratch and untested, you should really verify.


> Sorry for this long mail,but I thought it's better to see the whole
> former code, I hope you saw all my questions.

I hope I did ;-)  inserting a blank line between quoted and own text
helps readibility..

  Eike

-- 
 PGP/OpenPGP/GnuPG encrypted mail preferred in all private communication.
 Key ID: 0x293C05FD - 997A 4C60 CE41 0149 0DB3  9E96 2F1A D073 293C 05FD
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20110829/3a1664b1/attachment.pgp>