<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hello Eike,<br>
<br>
Let me to have some questions about your review:<br>
<br>
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?<br>
<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.<br>
<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.<br>
<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.<br>
<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.<br>
<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>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<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.<br>
<br>
Thank you for your help:<br>
<br>
Gabor<br>
</body>
</html>