Re: QCString construction

On Sunday 11 February 2007, Ingo Klöcker 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).
> 
> 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 size, 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.
> 
> I am very uneasy about those changes because completely
different things 
> happen when you copy a message and when you create a
message from a 
> string. 
Yeah, same here, that's what I asked.

> If we had unit tests... But as it stands I'm against
those  
> 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::autoSaveMessage
are a simple, safe, and huge improvement in memory usage
(found that one because 
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 calling 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 the 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.

-- 
David Faure, faurekde.org, dfaureklaralvdalens-datakonsult.se
KDE/KOffice developer, Qt consultancy projects
Klarälvdalens Datakonsult AB, Platform-independent software
solutions

_______________________________________________
Kde-optimize mailing list
Kde-optimizekde.org
ht
tps://mail.kde.org/mailman/listinfo/kde-optimize

On Monday 12 February 2007 16:21, David Faure wrote:
> On Sunday 11 February 2007, Ingo Klöcker wrote:
> > On Saturday 10 February 2007 02:16, David Faure
wrote:
> > > 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.
> >
> > I am very uneasy about those changes because
completely different
> > things happen when you copy a message and when you
create a message
> > from a string.
>
> Yeah, same here, that's what I asked.
>
> > If we had unit tests... But as it stands I'm
against those
> > changes.
>
> OK I made it at least
>   KMMessage msg;
>   msg.fromDwString(asDwString());
> which saves a DwString -> QCString -> DwString
roundtrip.

Good.

> The attached patch has many similar improvements.
> Things like calling asDwString instead of asString in
> KMComposeWin::autoSaveMessage are a simple, safe, and
huge
> improvement in memory usage (found that one because 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 calling 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 the 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.

There is a bug report about KMail truncating messages at the
first . 
So, yeah, I agree it's (part of) a fix for this bug.

The following doesn't look right:
+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;

I think it should be
+  memcpy( cstr.data(), str.data(), strLen );

Everything else looks good.

Ideally, we'd probably change all usage of QCString to
DwString. But I'm 
not sure whether it's worth putting much effort into this
since for KDE 
4.x we'll probably use QByteArray (with KMime instead of
mimelib) 
everywhere.

Regards,
Ingo

_______________________________________________
Kde-optimize mailing list
Kde-optimizekde.org
ht
tps://mail.kde.org/mailman/listinfo/kde-optimize