Re: QCString construction

--Boundary-00=_nYI0Feocqr1p3d6
Content-Type: text/plain;
  charset="iso-8859-15"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline

On Sunday 11 February 2007, Ingo Kl=F6cker wrote:
> On Saturday 10 February 2007 02:16, David Faure wrote:
> > This leads me to 3 patches.
> >
> > One for kmail, I wrote a utility function for creating a QCString
> > from char*+size, and used that when creating a QCString from a
> > DwString where it matters (i.e. where I found pretty large strings to
> > be used when attaching large files).
>=20
> The related changes look good. I'm just wondering why you didn't add
>   QCString CString( const DwString & str )
> instead of (or additionally to)
>   QCString CString( const char* str, size_t strLen )
> ?

You're right, it makes the code simpler. This came from my unit test which =
was DwString-independent,
and from the idea that maybe we have more cases were we know the string siz=
e, but in fact we don't.

> > Can a kmail developer review the change to KMMessage::asString() and
> > asSendableString(), too? It avoids a asString() (Assemble) and a
> > fromString (Parse), but I hope it's doing the right thing.
>=20
> I am very uneasy about those changes because completely different things=
=20
> happen when you copy a message and when you create a message from a=20
> string.=20
Yeah, same here, that's what I asked.

> If we had unit tests... But as it stands I'm against those =20
> changes.
OK I made it at least
  KMMessage msg;
  msg.fromDwString(asDwString());
which saves a DwString -> QCString -> DwString roundtrip.

The attached patch has many similar improvements.
Things like calling asDwString instead of asString in KMComposeWin::autoSav=
eMessage
are a simple, safe, and huge improvement in memory usage (found that one be=
cause=20
I got an "out of memory" abort from that code, after attaching a large file=
). Then I grepped
for other calls to asString and converted the relevant ones too.

The only part I'm a bit unsure about is strings with embedded nuls. E.g. in=
 bodyDecoded()
they would already trigger a kdWarning, which I commented out because calli=
ng the slow
QCString::length() just for that isn't worth it, but this means that later =
on when using size()-1
instead of length() we actually get the full data instead of stopping at th=
e first nil... Sounds like
a bugfix rather than a bug, but it's certainly a behavior change. Not sure =
how to trigger it
to test the side effects though.

=2D-=20
David Faure, [email protected], [email protected]
KDE/KOffice developer, Qt consultancy projects
Klar=E4lvdalens Datakonsult AB, Platform-independent software solutions

--Boundary-00=_nYI0Feocqr1p3d6
Content-Type: text/x-diff;
  charset="iso-8859-15";
  name="kmail.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
	filename="kmail.diff"

Index: util.cpp
===================================================================
--- util.cpp	(revision 632791)
+++ util.cpp	(working copy)
@@ -39,6 +39,7 @@
 
 #include <stdlib.h>
 #include <qcstring.h>
+#include <mimelib/string.h>
 
 size_t KMail::Util::crlf2lf( char* str, const size_t strLen )
 {
@@ -87,3 +88,12 @@ QCString KMail::Util::lf2crlf( const QCS
     result.truncate( d - result.begin() ); // adds trailing NUL
     return result;
 }
+
+QCString KMail::Util::CString( const DwString& str )
+{
+  const int strLen = str.size();
+  QCString cstr( strLen + 1 );
+  memcpy( cstr.data(), str.data(), strLen + 1 );
+  cstr[ strLen ] = 0;
+  return cstr;
+}
Index: kmmsgpart.cpp
===================================================================
--- kmmsgpart.cpp	(revision 632791)
+++ kmmsgpart.cpp	(working copy)
@@ -10,6 +10,7 @@
 #include "kmkernel.h"
 #include "kmmessage.h"
 #include "globalsettings.h"
+#include "util.h"
 
 #include <kasciistringtools.h>
 #include <kmime_charfreq.h>
@@ -102,7 +103,18 @@ int KMMessagePart::decodedSize(void) con
 //-----------------------------------------------------------------------------
 void KMMessagePart::setBody(const QCString &aStr)
 {
-  mBody.duplicate( aStr.data(), aStr.length() );
+  KMail::Util::setFromQCString( mBody, aStr );
+
+  int enc = cte();
+  if (enc == DwMime::kCte7bit || enc == DwMime::kCte8bit || enc == DwMime::kCteBinary)
+    mBodyDecodedSize = mBody.size();
+  else
+    mBodyDecodedSize = -1; // Can't know the decoded size
+}
+
+void KMMessagePart::setBody(const DwString &aStr)
+{
+  mBody.duplicate( aStr.c_str(), aStr.length() );
 
   int enc = cte();
   if (enc == DwMime::kCte7bit || enc == DwMime::kCte8bit || enc == DwMime::kCteBinary)
@@ -161,8 +173,7 @@ void KMMessagePart::setCharset( const QC
 //-----------------------------------------------------------------------------
 void KMMessagePart::setBodyEncoded(const QCString& aStr)
 {
-  mBodyDecodedSize = aStr.length();
-
+  mBodyDecodedSize = aStr.size() - 1; // same as aStr.length() but faster - assuming no embedded nuls
   switch (cte())
   {
   case DwMime::kCteQuotedPrintable:
@@ -224,7 +235,7 @@ void KMMessagePart::setBodyAndGuessCte(c
 				       bool allow8Bit,
                                        bool willBeSigned )
 {
-  mBodyDecodedSize = aBuf.length();
+  mBodyDecodedSize = aBuf.size() - 1; // same as aStr.length() but faster - assuming no embedded nuls
 
   CharFreq cf( aBuf.data(), mBodyDecodedSize ); // save to pass null strings
 
@@ -348,13 +359,12 @@ QCString KMMessagePart::bodyDecoded(void
 
   if ( decodeBinary ) {
     len = mBody.size();
-    result.resize( len+1 /* trailing NUL */ );
-    memcpy(result.data(), mBody.data(), len);
-    result[len] = 0;
+    KMail::Util::setFromByteArray( result, mBody );
   }
 
-  kdWarning( result.length() != (unsigned int)len, 5006 )
-    << "KMMessagePart::bodyDecoded(): body is binary but used as text!" << endl;
+  // Calls length -> slow
+  //kdWarning( result.length() != (unsigned int)len, 5006 )
+  //  << "KMMessagePart::bodyDecoded(): body is binary but used as text!" << endl;
 
   result = result.replace( "\r\n", "\n" ); // CRLF -> LF conversion
 
@@ -544,10 +554,12 @@ QString KMMessagePart::fileName(void) co
   return QString::null;
 }
 
-
-
 QCString KMMessagePart::body() const
 {
   return QCString( mBody.data(), mBody.size() + 1 ); // space for trailing NUL
 }
 
+DwString KMMessagePart::dwBody() const
+{
+  return DwString( mBody.data(), mBody.size() );
+}
Index: kmfoldermaildir.cpp
===================================================================
--- kmfoldermaildir.cpp	(revision 632791)
+++ kmfoldermaildir.cpp	(working copy)
@@ -406,7 +406,7 @@ if( fileD0.open( IO_WriteOnly ) ) {
   if ( !uidHeader.isEmpty() && stripUid )
     aMsg->removeHeaderField( "X-UID" );
 
-  msgText = aMsg->asString();
+  msgText = aMsg->asString(); // TODO use asDwString instead
   len = msgText.length();
 
   // Re-add the uid so that the take can make use of it, in case the
Index: kmreaderwin.cpp
===================================================================
--- kmreaderwin.cpp	(revision 632791)
+++ kmreaderwin.cpp	(working copy)
@@ -1614,13 +1614,13 @@ kdDebug(5006) << "KMReaderWin  -  compos
       unencryptedMessage->setParent( 0 );
       // because this did not work:
       /*
-      DwMessage dwMsg( DwString( aMsg->asString() ) );
+      DwMessage dwMsg( aMsg->asDwString() );
       dwMsg.Body() = DwBody( DwString( resultString.data() ) );
       dwMsg.Body().Parse();
       KMMessage* unencryptedMessage = new KMMessage( &dwMsg );
       */
-kdDebug(5006) << "KMReaderWin  -  resulting message:" << unencryptedMessage->asString() << endl;
-kdDebug(5006) << "KMReaderWin  -  attach unencrypted message to aMsg" << endl;
+      //kdDebug(5006) << "KMReaderWin  -  resulting message:" << unencryptedMessage->asString() << endl;
+      kdDebug(5006) << "KMReaderWin  -  attach unencrypted message to aMsg" << endl;
       aMsg->setUnencryptedMsg( unencryptedMessage );
       emitReplaceMsgByUnencryptedVersion = true;
     }
Index: partNode.cpp
===================================================================
--- partNode.cpp	(revision 632791)
+++ partNode.cpp	(working copy)
@@ -37,6 +37,7 @@
 #include <mimelib/utility.h>
 #include <qregexp.h>
 #include <kasciistricmp.h>
+#include "util.h"
 
 /*
   ===========================================================================
@@ -179,7 +180,7 @@ const QCString & partNode::encodedBody()
     return mEncodedBody;
 
   if ( mDwPart )
-    mEncodedBody = mDwPart->Body().AsString().c_str();
+    mEncodedBody = KMail::Util::CString( mDwPart->Body().AsString() );
   else
     mEncodedBody = 0;
   mEncodedOk = true;
Index: kmmessage.cpp
===================================================================
--- kmmessage.cpp	(revision 632791)
+++ kmmessage.cpp	(working copy)
@@ -64,6 +64,7 @@ using namespace KMime::Types;
 #include <klocale.h>
 #include <stdlib.h>
 #include <unistd.h>
+#include "util.h"
 
 #if ALLOW_GUI
 #include <kmessagebox.h>
@@ -212,7 +213,7 @@ QCString KMMessage::id() const
 {
   DwHeaders& header = mMsg->Headers();
   if (header.HasMessageId())
-    return header.MessageId().AsString().c_str();
+    return KMail::Util::CString( header.MessageId().AsString() );
   else
     return "";
 }
@@ -306,14 +307,14 @@ const DwMessage *KMMessage::asDwMessage(
 
 //-----------------------------------------------------------------------------
 QCString KMMessage::asString() const {
-  return asDwString().c_str();
+  return KMail::Util::CString( asDwString() );
 }
 
 
 QCString KMMessage::asSendableString() const
 {
   KMMessage msg;
-  msg.fromString(asString());
+  msg.fromDwString(asDwString()); // slow!
   msg.removePrivateHeaderFields();
   msg.removeHeaderField("Bcc");
   return msg.asString();
@@ -322,7 +323,7 @@ QCString KMMessage::asSendableString() c
 QCString KMMessage::headerAsSendableString() const
 {
   KMMessage msg;
-  msg.fromString(asString());
+  msg.fromDwString(asDwString());
   msg.removePrivateHeaderFields();
   msg.removeHeaderField("Bcc");
   return msg.headerAsString().latin1();
@@ -392,7 +393,7 @@ void KMMessage::fromByteArray( const QBy
 }
 
 void KMMessage::fromString( const QCString & str, bool aSetStatus ) {
-  return fromDwString( DwString( str.data() ), aSetStatus );
+  return fromDwString( DwString( str.data(), str.size() ), aSetStatus );
 }
 
 void KMMessage::fromDwString(const DwString& str, bool aSetStatus)
@@ -1250,7 +1251,7 @@ KMMessage* KMMessage::createForward( con
     KMMessagePart secondPart;
     secondPart.setType( type() );
     secondPart.setSubtype( subtype() );
-    secondPart.setBody( mMsg->Body().AsString().c_str() );
+    secondPart.setBody( mMsg->Body().AsString() );
     // use the headers of the original mail
     applyHeadersToMessagePart( mMsg->Headers(), &secondPart );
     msg->addBodyPart(&secondPart);
@@ -1761,9 +1762,9 @@ QCString KMMessage::dateShortStr() const
   unixTime = header.Date().AsUnixTime();
 
   QCString result = ctime(&unixTime);
-
-  if (result[result.length()-1]=='\n')
-    result.truncate(result.length()-1);
+  int len = result.length();
+  if (result[len-1]=='\n')
+    result.truncate(len-1);
 
   return result;
 }
@@ -2469,10 +2470,11 @@ void KMMessage::setNeedsAssembly()
 //-----------------------------------------------------------------------------
 QCString KMMessage::body() const
 {
-  DwString body = mMsg->Body().AsString();
-  QCString str = body.c_str();
-  kdWarning( str.length() != body.length(), 5006 )
-    << "KMMessage::body(): body is binary but used as text!" << endl;
+  const DwString& body = mMsg->Body().AsString();
+  QCString str = KMail::Util::CString( body );
+  // Calls length() -> slow
+  //kdWarning( str.length() != body.length(), 5006 )
+  //  << "KMMessage::body(): body is binary but used as text!" << endl;
   return str;
 }
 
@@ -2481,7 +2483,7 @@ QCString KMMessage::body() const
 QByteArray KMMessage::bodyDecodedBinary() const
 {
   DwString dwstr;
-  DwString dwsrc = mMsg->Body().AsString();
+  const DwString& dwsrc = mMsg->Body().AsString();
 
   switch (cte())
   {
@@ -2522,13 +2524,13 @@ QCString KMMessage::bodyDecoded() const
     break;
   }
 
-  unsigned int len = dwstr.size();
-  QCString result(len+1);
-  memcpy(result.data(),dwstr.data(),len);
-  result[len] = 0;
-  kdWarning(result.length() != len, 5006)
-    << "KMMessage::bodyDecoded(): body is binary but used as text!" << endl;
-  return result;
+  return KMail::Util::CString( dwstr );
+
+  // Calling QCString::length() is slow
+  //QCString result = KMail::Util::CString( dwstr );
+  //kdWarning(result.length() != len, 5006)
+  //  << "KMMessage::bodyDecoded(): body is binary but used as text!" << endl;
+  //return result;
 }
 
 
@@ -2609,7 +2611,7 @@ void KMMessage::setBodyAndGuessCte( cons
                                     bool allow8Bit,
                                     bool willBeSigned )
 {
-  CharFreq cf( aBuf.data(), aBuf.length() ); // it's safe to pass null strings
+  CharFreq cf( aBuf.data(), aBuf.size()-1 ); // it's safe to pass null strings
 
   allowedCte = determineAllowedCtes( cf, allow8Bit, willBeSigned );
 
@@ -2964,9 +2966,9 @@ void KMMessage::bodyPart(DwBodyPart* aDw
 
     // Body
     if (withBody)
-      aPart->setBody( aDwBodyPart->Body().AsString().c_str() );
+      aPart->setBody( aDwBodyPart->Body().AsString() );
     else
-      aPart->setBody( "" );
+      aPart->setBody( QCString("") );
 
     // Content-id
     if ( headers.HasContentId() ) {
@@ -2987,7 +2989,7 @@ void KMMessage::bodyPart(DwBodyPart* aDw
     //aPart->setName(" ");
     aPart->setContentDescription("");
     aPart->setContentDisposition("");
-    aPart->setBody("");
+    aPart->setBody(QCString(""));
     aPart->setContentId("");
   }
 }
@@ -3135,8 +3137,9 @@ DwBodyPart* KMMessage::createDWBodyPart(
   if (!contDisp.isEmpty())
     headers.ContentDisposition().FromString(contDisp);
 
-  if (!aPart->body().isNull())
-    part->Body().FromString(aPart->body());
+  const DwString bodyStr = aPart->dwBody();
+  if (!bodyStr.empty())
+    part->Body().FromString(bodyStr);
   else
     part->Body().FromString("");
 
@@ -3194,7 +3197,7 @@ QString KMMessage::generateMessageId( co
 //-----------------------------------------------------------------------------
 QCString KMMessage::html2source( const QCString & src )
 {
-  QCString result( 1 + 6*src.length() );  // maximal possible length
+  QCString result( 1 + 6*(src.size()-1) );  // maximal possible length
 
   QCString::ConstIterator s = src.begin();
   QCString::Iterator d = result.begin();
Index: kmcommands.cpp
===================================================================
--- kmcommands.cpp	(revision 632791)
+++ kmcommands.cpp	(working copy)
@@ -692,7 +692,7 @@ KMCommand::Result KMUseTemplateCommand::
   // Take a copy of the original message, which remains unchanged.
   KMMessage *newMsg = new KMMessage;
   newMsg->setComplete( msg->isComplete() );
-  newMsg->fromString( msg->asString() );
+  newMsg->fromDwString( msg->asDwString() );
 
   KMail::Composer *win = KMail::makeComposer();
   newMsg->setTransferInProgress( false ); // From here on on, the composer owns the message.
@@ -1923,7 +1923,7 @@ KMCommand::Result KMCopyCommand::execute
       // make sure the attachment state is only calculated when it's complete
       if (!newMsg->isComplete())
         newMsg->setReadyToShow(false);
-      newMsg->fromString(msg->asString());
+      newMsg->fromDwString(msg->asDwString());
       newMsg->setStatus(msg->status());
 
       if (srcFolder && !newMsg->isComplete())
Index: messagecomposer.cpp
===================================================================
--- messagecomposer.cpp	(revision 632791)
+++ messagecomposer.cpp	(working copy)
@@ -1548,7 +1548,7 @@ void MessageComposer::composeMessage( KM
       if ( it->encrypt == doEncryptBody && it->sign == doSignBody ) {
         innerDwPart = theMessage.createDWBodyPart( it->part );
         innerDwPart->Assemble();
-        body += "\n--" + mMultipartMixedBoundary + "\n" + innerDwPart->AsString().c_str();
+        body += "\n--" + mMultipartMixedBoundary + "\n" + innerDwPart->AsString().c_str(); // SLOW!
         delete innerDwPart;
         innerDwPart = 0;
       }
@@ -1577,13 +1577,12 @@ void MessageComposer::composeMessage( KM
       DwMediaType& ct = headers.ContentType();
       ct.SetBoundary(mSaveBoundary);
       dwPart->Assemble();
-      mEncodedBody = dwPart->AsString().c_str();
     }
     else {
       dwPart = theMessage.createDWBodyPart( &mOldBodyPart );
       dwPart->Assemble();
-      mEncodedBody = dwPart->AsString().c_str();
     }
+    mEncodedBody = KMail::Util::CString( dwPart->AsString() );
     delete dwPart;
     dwPart = 0;
 
@@ -1784,7 +1783,7 @@ void MessageComposer::addBodyAndAttachme
 
       DwBodyPart* innerDwPart = msg->createDWBodyPart( it->part );
       innerDwPart->Assemble();
-      QCString encodedAttachment = innerDwPart->AsString().c_str();
+      QCString encodedAttachment = KMail::Util::CString( innerDwPart->AsString() );
       delete innerDwPart;
       innerDwPart = 0;
 
@@ -1815,7 +1814,7 @@ void MessageComposer::addBodyAndAttachme
               rEncryptMessagePart = newAttachPart;
               DwBodyPart* dwPart = msg->createDWBodyPart( &newAttachPart );
               dwPart->Assemble();
-              encodedAttachment = dwPart->AsString().c_str();
+              encodedAttachment = KMail::Util::CString( dwPart->AsString() );
               delete dwPart;
               dwPart = 0;
             }
Index: kmcomposewin.cpp
===================================================================
--- kmcomposewin.cpp	(revision 632802)
+++ kmcomposewin.cpp	(working copy)
@@ -707,8 +707,8 @@ void KMComposeWin::autoSaveMessage()
   if ( status == 0 ) { // no error
     kdDebug(5006) << "autosaving message in " << filename << endl;
     int fd = autoSaveFile.handle();
-    QCString msgStr = msg->asString();
-    if ( ::write( fd, msgStr, msgStr.length() ) == -1 )
+    const DwString& msgStr = msg->asDwString();
+    if ( ::write( fd, msgStr.data(), msgStr.length() ) == -1 )
       status = errno;
   }
   if ( status == 0 ) {
Index: util.h
===================================================================
--- util.h	(revision 632791)
+++ util.h	(working copy)
@@ -41,6 +41,7 @@
 #include <stdlib.h>
 #include <qobject.h>
 #include <qcstring.h>
+class DwString;
 
 namespace KMail
 {
@@ -68,8 +69,39 @@ namespace Util {
     QCString lf2crlf( const QCString & src );
 
     /**
+     * Construct a QCString from a DwString
+     */
+    QCString CString( const DwString& str );
+
+    /**
+     * Fills a QByteArray from a QCString - removing the trailing null.
+     */
+    void setFromQCString( QByteArray& arr, const QCString& cstr );
+
+    inline void setFromQCString( QByteArray& arr, const QCString& cstr )
+    {
+      if ( cstr.size() )
+        arr.duplicate( cstr.data(), cstr.size()-1 );
+      else
+        arr.resize(0);
+    }
+
+    /**
+     * Fills a QCString from a QByteArray - adding the trailing null.
+     */
+    void setFromByteArray( QCString& cstr, const QByteArray& arr );
+
+    inline void setFromByteArray( QCString& result, const QByteArray& arr )
+    {
+      const int len = arr.size();
+      result.resize( len + 1 /* trailing NUL */ );
+      memcpy(result.data(), arr.data(), len);
+      result[len] = 0;
+    }
+
+   /**
      * A LaterDeleter is intended to be used with the RAII ( Resource
-     * Acquisiation is Initialization ) paradigm. When an instance of it
+     * Acquisition is Initialization ) paradigm. When an instance of it
      * goes out of scope it deletes the associated object  It can be 
      * disabled, in case the deletion needs to be avoided for some 
      * reason, since going out-of-scope cannot be avoided.
Index: kmmsgpart.h
===================================================================
--- kmmsgpart.h	(revision 632791)
+++ kmmsgpart.h	(working copy)
@@ -27,6 +27,7 @@
 template <typename T>
 class QValueList;
 class QTextCodec;
+class DwString;
 
 class KMMessagePart
 {
@@ -45,6 +46,8 @@ public:
   /** Get or set the message body */
   QCString body(void) const;
   void setBody(const QCString &aStr);
+  DwString dwBody() const;
+  void setBody(const DwString &aStr);
 
   /** Sets this body part's content to @p str. @p str is subject to
       automatic charset and CTE detection.

--Boundary-00=_nYI0Feocqr1p3d6
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
KMail developers mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/kmail-devel

--Boundary-00=_nYI0Feocqr1p3d6--