D15890: kimg_rgb: optimize away QRegExp and QString::fromLocal8Bit.

2018-10-02 Thread David Faure 
dfaure closed this revision.

REPOSITORY
  R287 KImageFormats

REVISION DETAIL
  https://phabricator.kde.org/D15890

To: dfaure, cfeck
Cc: jtamate, kde-frameworks-devel, michaelh, ngraham, bruns

D15890: kimg_rgb: optimize away QRegExp and QString::fromLocal8Bit.

2018-10-02 Thread Christoph Feck 
cfeck added inline comments.

INLINE COMMENTS

> jtamate wrote in rgb.cpp:702
> Shouldn't it be QLatin1String("\x01\xda\x01")?
> startsWith has a QLatin1String overloaded method, but will it be used if a 
> char* is used as argument or will it use the QString method? I just don't 
> know the answer.
> Otherwise, +1.

`head` is a QByteArray, not a QString.

REPOSITORY
  R287 KImageFormats

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D15890

To: dfaure, cfeck
Cc: jtamate, kde-frameworks-devel, michaelh, ngraham, bruns

D15890: kimg_rgb: optimize away QRegExp and QString::fromLocal8Bit.

2018-10-02 Thread Jaime Torres Amate 
jtamate added inline comments.

INLINE COMMENTS

> rgb.cpp:702
>  
> -const QRegExp regexp(QLatin1String("^\x01\xda\x01[\x01\x02]"));
> -QString data(QString::fromLocal8Bit(head));
> -
> -return data.contains(regexp);
> +return head.size() >= 4 && head.startsWith("\x01\xda\x01") && (head[3] 
> == 1 || head[3] == 2);
>  }

Shouldn't it be QLatin1String("\x01\xda\x01")?
startsWith has a QLatin1String overloaded method, but will it be used if a 
char* is used as argument or will it use the QString method? I just don't know 
the answer.
Otherwise, +1.

REPOSITORY
  R287 KImageFormats

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D15890

To: dfaure, cfeck
Cc: jtamate, kde-frameworks-devel, michaelh, ngraham, bruns

D15890: kimg_rgb: optimize away QRegExp and QString::fromLocal8Bit.

2018-10-01 Thread David Faure 
dfaure added inline comments.

INLINE COMMENTS

> cfeck wrote in rgb.cpp:702
> head[i] → head.at(i)

This would make no difference, given that I made `head` const...

REPOSITORY
  R287 KImageFormats

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D15890

To: dfaure, cfeck
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15890: kimg_rgb: optimize away QRegExp and QString::fromLocal8Bit.

2018-10-01 Thread Christoph Feck 
cfeck accepted this revision.
cfeck added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> rgb.cpp:702
>  
> -const QRegExp regexp(QLatin1String("^\x01\xda\x01[\x01\x02]"));
> -QString data(QString::fromLocal8Bit(head));
> -
> -return data.contains(regexp);
> +return head.size() >= 4 && head.startsWith("\x01\xda\x01") && (head[3] 
> == 1 || head[3] == 2);
>  }

head[i] → head.at(i)

REPOSITORY
  R287 KImageFormats

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D15890

To: dfaure, cfeck
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15890: kimg_rgb: optimize away QRegExp and QString::fromLocal8Bit.

2018-10-01 Thread David Faure 
dfaure created this revision.
dfaure added a reviewer: cfeck.
Herald added a project: Frameworks.
Herald edited subscribers, added: kde-frameworks-devel; removed: Frameworks.
dfaure requested review of this revision.

REVISION SUMMARY
  The code is even simpler this way.
  
  Found by using heaptrack.

TEST PLAN
  the unittest for rgb still passes.

REPOSITORY
  R287 KImageFormats

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D15890

AFFECTED FILES
  src/imageformats/rgb.cpp

To: dfaure, cfeck
Cc: kde-frameworks-devel, michaelh, ngraham, bruns