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
Attachment:
signature.asc
Description: Digital signature