Re: [Libreoffice] [PATCH] BUG 36594

2011-09-02 Thread Jenei Gábor 

Hello Michael,

I think Eike is already working on this problem, but finally we got that 
there is no really good solution, maybe my first advised option could be 
applied with success(storing for each comment if there is a query in the 
same line in a bool variable). I guess you should consult with Eike, but 
naturally if you can mind about the problem it is great also. Actually I 
won't mind too much in the future, but naturally if you have questions 
or suggestions for my patch I'll answer.


Gabor

2011. 09. 02. 22:37 keltezéssel, Michael Meeks írta:

Hi Jenei,

On Mon, 2011-08-29 at 22:45 +0200, Eike Rathke wrote:

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

Let me to have some questions about your review:

Wow - ... there was a lot of feedback here - hopefully not too
unfriendly ;-) it is really great to have you contributing, and your
work is much appreciated [ sometimes that can get lost in a long list of
minor glitches from Eike ].

Did you have any joy cleaning those up ? or would you like me to have a
look next week ? :-)

All the best,

Michael.



___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice] [PATCH] BUG 36594

2011-08-29 Thread Jenei Gábor 

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 vectorOUStringBuffer  ? vectorOUString  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;inQueryLen;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 vectorOUString  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 vectorOUString  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;isComments.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

Re: [Libreoffice] [PATCH] BUG 36594

2011-08-29 Thread 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;inQueryLen;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 vectorOUString  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;isComments.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 nIndLF0

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


signature.asc
Description: Digital signature
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice] [PATCH] BUG 36594

2011-08-22 Thread Eike Rathke 
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 vectorOUStringBuffer ? vectorOUString 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;inQueryLen;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 vectorOUString is returned, change that to

   sRet.push_back( sTemp.makeStringAndClear());

then there's also no need to assign sTemp an empty OUString anymore.

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

 +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 vectorOUString instead, as above.

 +::rtl::OUStringBuffer sRet;
 +int nIndLF=0;
 +int nIndBeg=0;
 +for(unsigned int i=0;isComments.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.

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

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

-- 
 PGP/OpenPGP/GnuPG encrypted mail preferred in all private communication.
 Key ID: 0x293C05FD - 997A 4C60 CE41 0149 0DB3  9E96 2F1A D073 293C 05FD


signature.asc
Description: Digital signature
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice