[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: Review kdelibs whiting/fixQByteArrays
From:       Jeremy Whiting <jpwhiting () kde ! org>
Date:       2011-05-12 4:02:15
Message-ID: BANLkTim8imFLQmrUSZwviPmOUSESaBNWSw () mail ! gmail ! com
[Download RAW message or body]

Ok, I cleaned this up, redid it basically with the suggestions from this
thread.  I put a review request on reviewboard, and the code is in kdelibs
whiting/buildwithqt48 branch.

Enjoy,
Jeremy

On Mon, May 2, 2011 at 11:37 AM, Olivier Goffart <ogoffart@kde.org> wrote:

> Le Monday 02 May 2011, David Faure a écrit :
>
> > Another one:
> >
> > -      envs << QString::fromLatin1( QByteArray("DISPLAY=") + dpystring );
> > +    envs << QString::fromLatin1( QByteArray(QByteArray("DISPLAY=") +
> > dpystring) );
>
> Should be QLatin1String("DISPLAY=") + QLatin1String(dpystring)
> (Assuming dpystring is char*)
>
>
> >
> > Is this still because QString::fromLatin1( const QByteArray& ) is
> missing,
> > so it has to cast to const char* here too? Olivier, wouldn't it make
> sense
> > to have such an overload? (It would save a strlen, too).
>
> Yes, maybe  QString::fromLatin1(const QByteArray&) would make sens
> But it might brake source compatibility if the call becomes ambiguous.
>
> The reason why we brake source compatibility with QT_USE_FAST_OPERATOR_PLUS
> is
> because this one was not documented.  But maybe the new operator should
> only
> be enabled if QT_USE_FAST_OPERATOR_PLUS is defined to 2 or something.
>
>
>
>
>

[Attachment #3 (text/html)]

Ok, I cleaned this up, redid it basically with the suggestions from this thread.  I \
put a review request on reviewboard, and the code is in kdelibs whiting/buildwithqt48 \
branch.<br><br>Enjoy,<br>Jeremy<br><br><div class="gmail_quote"> On Mon, May 2, 2011 \
at 11:37 AM, Olivier Goffart <span dir="ltr">&lt;<a \
href="mailto:ogoffart@kde.org">ogoffart@kde.org</a>&gt;</span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex;"> Le Monday 02 May 2011, David Faure a écrit :<br>
<div class="im"><br>
&gt; Another one:<br>
&gt;<br>
&gt; -      envs &lt;&lt; QString::fromLatin1( QByteArray(&quot;DISPLAY=&quot;) + \
dpystring );<br> &gt; +    envs &lt;&lt; QString::fromLatin1( \
QByteArray(QByteArray(&quot;DISPLAY=&quot;) +<br> &gt; dpystring) );<br>
<br>
</div>Should be QLatin1String(&quot;DISPLAY=&quot;) + QLatin1String(dpystring)<br>
(Assuming dpystring is char*)<br>
<div class="im"><br>
<br>
&gt;<br>
&gt; Is this still because QString::fromLatin1( const QByteArray&amp; ) is \
missing,<br> &gt; so it has to cast to const char* here too? Olivier, wouldn&#39;t it \
make sense<br> &gt; to have such an overload? (It would save a strlen, too).<br>
<br>
</div>Yes, maybe  QString::fromLatin1(const QByteArray&amp;) would make sens<br>
But it might brake source compatibility if the call becomes ambiguous.<br>
<br>
The reason why we brake source compatibility with QT_USE_FAST_OPERATOR_PLUS is<br>
because this one was not documented.  But maybe the new operator should only<br>
be enabled if QT_USE_FAST_OPERATOR_PLUS is defined to 2 or something.<br>
<br>
<br>
<br>
<br>
</blockquote></div><br>



[prev in list] [next in list] [prev in thread] [next in thread]