<html> <head> <meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> Hello Eike, Let me to have some questions about your review: 2011. 08. 22. 21:47 keltezéssel, Eike Rathke írta: <blockquote cite="mid:20110822194757.GD14528@kulungile.erack.de" type="cite"> <pre wrap="">Hi Jenei, On Sunday, 2011-08-21 14:59:26 +0200, Jenei Gábor wrote: </pre> <blockquote type="cite"> <pre wrap="">+ ::rtl::OUStringBuffer sTemp; </pre> </blockquote> <pre wrap=""> Construct the buffer with a sufficient capacity beforehand so no memory allocation needs to be done in between: ::rtl::OUStringBuffer sTemp( nQueryLen); </pre> <blockquote type="cite"> <pre wrap="">[...] + return (::rtl::OUString)sTemp; </pre> </blockquote> <pre wrap=""> 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(). </pre> <blockquote type="cite"> <pre wrap="">+std::vector< ::rtl::OUStringBuffer > getComment(const ::rtl::OUString& sQuery){ </pre> </blockquote> <pre wrap=""> Why return vector<OUStringBuffer> ? vector<OUString> seems to be more straight forward. </pre> <blockquote type="cite"> <pre wrap="">+ 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'); </pre> </blockquote> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ sRet.push_back(sTemp); + sTemp=::rtl::OUString(); </pre> </blockquote> <pre wrap=""> 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.</pre> </blockquote> 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? <blockquote cite="mid:20110822194757.GD14528@kulungile.erack.de" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ } + 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(); </pre> </blockquote> <pre wrap=""> That one is superfluous, sTemp as a stack local variable will be destructed upon return anyway.</pre> </blockquote> yes,thank you,this is a needless line in the end. <blockquote cite="mid:20110822194757.GD14528@kulungile.erack.de" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ return sRet; +} </pre> </blockquote> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+//------------------------------------------------------------------------------ +::rtl::OUString ConcatComment(const ::rtl::OUString& sQuery,std::vector< ::rtl::OUStringBuffer > sComments){ </pre> </blockquote> <pre wrap=""> Pass sComments as const reference so it doesn't need to be copied when ConcatComment() is called. And use vector<OUString> instead, as above.</pre> </blockquote> Yes,this is also ok, I've already modified it, passing classes by value may need long time and memory. <blockquote cite="mid:20110822194757.GD14528@kulungile.erack.de" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ ::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(); </pre> </blockquote> <pre wrap=""> 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.</pre> </blockquote> 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. <blockquote cite="mid:20110822194757.GD14528@kulungile.erack.de" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ } + else{ + nIndLF=sQuery.indexOf('\n',nIndLF); + } + sRet.append(sQuery.copy(nIndBeg,nIndLF-nIndBeg)); </pre> </blockquote> <pre wrap=""> 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.</pre> </blockquote> 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. <blockquote cite="mid:20110822194757.GD14528@kulungile.erack.de" type="cite"> <pre wrap=""> 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)); </pre> <blockquote type="cite"> <pre wrap="">+ sRet.append(sComments[i]); + } + return (::rtl::OUString)sRet; </pre> </blockquote> <pre wrap=""> Same here, use return sRet.makeStringAndClear(); Btw, </pre> <blockquote type="cite"> <pre wrap="">+ sTranslatedStmt=ConcatComment(sTranslatedStmt,sComments); </pre> </blockquote> <pre wrap=""> blanks increase legibility, maybe it's just me but I'd much prefer a sTranslatedStmt = ConcatComment( sTranslatedStmt, sComments); style instead. Thanks Eike </pre> <fieldset class="mimeAttachmentHeader"></fieldset> <pre wrap="">_______________________________________________ LibreOffice mailing list <a class="moz-txt-link-abbreviated" href="mailto:LibreOffice@lists.freedesktop.org">LibreOffice@lists.freedesktop.org</a> <a class="moz-txt-link-freetext" href="http://lists.freedesktop.org/mailman/listinfo/libreoffice">http://lists.freedesktop.org/mailman/listinfo/libreoffice</a> </pre> </blockquote> 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 </body> </html>