Re: Review Request 126148: [plasma-framework] qstring optimizations

2015-11-27 Thread Nick Shaforostoff 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126148/
---

(Updated Nov. 27, 2015, 8:19 p.m.)


Status
--

This change has been marked as submitted.


Review request for Plasma, Marco Martin and Martin Klapetek.


Changes
---

Submitted with commit c7c2980f1429e0b0972b8754c37a2c4cd4ef5233 by Nick 
Shaforostoff to branch master.


Repository: plasma-framework


Description
---

also contains fix for clazy warning regarding iterating over temporary 
roleNames() object

note that for QRegExp constructor and for QIcon::fromTheme() i used 
QLatin1String because QStringLiteral is known to produce crashes-on-exit if the 
code is inside a library


Diffs
-

  CMakeLists.txt 812dc96 
  src/declarativeimports/core/datamodel.cpp 1c83bc5 
  src/declarativeimports/core/framesvgitem.cpp 9aab228 
  src/declarativeimports/core/iconitem.cpp f32b3d1 
  src/plasma/applet.cpp 89c 
  src/plasma/containment.cpp 1d166f2 
  src/plasma/framesvg.cpp f9379b0 
  src/plasma/pluginloader.cpp bfa5dee 
  src/plasma/private/applet_p.cpp e935d52 
  src/plasma/private/containment_p.cpp 3836772 
  src/plasma/private/theme_p.cpp 7bf81d4 
  src/plasma/scripting/scriptengine.cpp 5b2cf84 
  src/plasma/svg.cpp 20cb31f 
  src/plasma/theme.cpp c49ad4c 
  src/plasmaquick/appletquickitem.cpp efe8611 
  src/plasmaquick/shellpluginloader.cpp 2824760 

Diff: https://git.reviewboard.kde.org/r/126148/diff/


Testing
---

compiles fine


Thanks,

Nick Shaforostoff

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126148: [plasma-framework] qstring optimizations

2015-11-25 Thread Marco Martin 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126148/#review88822
---

Ship it!


Ship It!

- Marco Martin


On Nov. 23, 2015, 10:19 p.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126148/
> ---
> 
> (Updated Nov. 23, 2015, 10:19 p.m.)
> 
> 
> Review request for Plasma, Marco Martin and Martin Klapetek.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> also contains fix for clazy warning regarding iterating over temporary 
> roleNames() object
> 
> note that for QRegExp constructor and for QIcon::fromTheme() i used 
> QLatin1String because QStringLiteral is known to produce crashes-on-exit if 
> the code is inside a library
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 812dc96 
>   src/declarativeimports/core/datamodel.cpp 1c83bc5 
>   src/declarativeimports/core/framesvgitem.cpp 9aab228 
>   src/declarativeimports/core/iconitem.cpp f32b3d1 
>   src/plasma/applet.cpp 89c 
>   src/plasma/containment.cpp 1d166f2 
>   src/plasma/framesvg.cpp f9379b0 
>   src/plasma/pluginloader.cpp bfa5dee 
>   src/plasma/private/applet_p.cpp e935d52 
>   src/plasma/private/containment_p.cpp 3836772 
>   src/plasma/private/theme_p.cpp 7bf81d4 
>   src/plasma/scripting/scriptengine.cpp 5b2cf84 
>   src/plasma/svg.cpp 20cb31f 
>   src/plasma/theme.cpp c49ad4c 
>   src/plasmaquick/appletquickitem.cpp efe8611 
>   src/plasmaquick/shellpluginloader.cpp 2824760 
> 
> Diff: https://git.reviewboard.kde.org/r/126148/diff/
> 
> 
> Testing
> ---
> 
> compiles fine
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126148: [plasma-framework] qstring optimizations

2015-11-24 Thread Marco Martin 


> On Nov. 23, 2015, 9:53 p.m., Kai Uwe Broulik wrote:
> > src/declarativeimports/core/datamodel.cpp, line 58
> > 
> >
> > Why not constBegin()/constEnd()?
> 
> Nick Shaforostoff wrote:
> because rNames is const, constBegin()/constEnd() will be called internaly 
> anyways, and begin()/end() just look nicer

if it ends up being the same thing, i would prefer it more explicit with 
constbegin/constend


- Marco


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126148/#review88737
---


On Nov. 23, 2015, 10:19 p.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126148/
> ---
> 
> (Updated Nov. 23, 2015, 10:19 p.m.)
> 
> 
> Review request for Plasma, Marco Martin and Martin Klapetek.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> also contains fix for clazy warning regarding iterating over temporary 
> roleNames() object
> 
> note that for QRegExp constructor and for QIcon::fromTheme() i used 
> QLatin1String because QStringLiteral is known to produce crashes-on-exit if 
> the code is inside a library
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 812dc96 
>   src/declarativeimports/core/datamodel.cpp 1c83bc5 
>   src/declarativeimports/core/framesvgitem.cpp 9aab228 
>   src/declarativeimports/core/iconitem.cpp f32b3d1 
>   src/plasma/applet.cpp 89c 
>   src/plasma/containment.cpp 1d166f2 
>   src/plasma/framesvg.cpp f9379b0 
>   src/plasma/pluginloader.cpp bfa5dee 
>   src/plasma/private/applet_p.cpp e935d52 
>   src/plasma/private/containment_p.cpp 3836772 
>   src/plasma/private/theme_p.cpp 7bf81d4 
>   src/plasma/scripting/scriptengine.cpp 5b2cf84 
>   src/plasma/svg.cpp 20cb31f 
>   src/plasma/theme.cpp c49ad4c 
>   src/plasmaquick/appletquickitem.cpp efe8611 
>   src/plasmaquick/shellpluginloader.cpp 2824760 
> 
> Diff: https://git.reviewboard.kde.org/r/126148/diff/
> 
> 
> Testing
> ---
> 
> compiles fine
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126148: [plasma-framework] qstring optimizations

2015-11-23 Thread Nick Shaforostoff 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126148/
---

(Updated Nov. 23, 2015, 10:19 p.m.)


Review request for Plasma, Marco Martin and Martin Klapetek.


Changes
---

removed QStringLiteral use for LoadPackage calls becaus they may store the 
string argument in the static variable


Repository: plasma-framework


Description
---

also contains fix for clazy warning regarding iterating over temporary 
roleNames() object

note that for QRegExp constructor and for QIcon::fromTheme() i used 
QLatin1String because QStringLiteral is known to produce crashes-on-exit if the 
code is inside a library


Diffs (updated)
-

  CMakeLists.txt 812dc96 
  src/declarativeimports/core/datamodel.cpp 1c83bc5 
  src/declarativeimports/core/framesvgitem.cpp 9aab228 
  src/declarativeimports/core/iconitem.cpp f32b3d1 
  src/plasma/applet.cpp 89c 
  src/plasma/containment.cpp 1d166f2 
  src/plasma/framesvg.cpp f9379b0 
  src/plasma/pluginloader.cpp bfa5dee 
  src/plasma/private/applet_p.cpp e935d52 
  src/plasma/private/containment_p.cpp 3836772 
  src/plasma/private/theme_p.cpp 7bf81d4 
  src/plasma/scripting/scriptengine.cpp 5b2cf84 
  src/plasma/svg.cpp 20cb31f 
  src/plasma/theme.cpp c49ad4c 
  src/plasmaquick/appletquickitem.cpp efe8611 
  src/plasmaquick/shellpluginloader.cpp 2824760 

Diff: https://git.reviewboard.kde.org/r/126148/diff/


Testing
---

compiles fine


Thanks,

Nick Shaforostoff

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126148: [plasma-framework] qstring optimizations

2015-11-23 Thread Nick Shaforostoff 


> On Nov. 23, 2015, 9:02 p.m., Aleix Pol Gonzalez wrote:
> > CMakeLists.txt, line 122
> > 
> >
> > Why do you drop FAST_CONCATENATION?

ack QT_USE_FAST_CONCATENATION /usr/include/ brings nothing
ack QT_USE_QSTRINGBUILDER /usr/include/ brings real results


On Nov. 23, 2015, 9:02 p.m., Nick Shaforostoff wrote:
> > Regarding QStringLiteral vs QLatin1String for libraries: I read that 
> > thread, but then most of QStringLiterals here sould be dropped as well if 
> > that was the case. I don't think it makes sense to make a special case of 
> > QIcon::fromTheme.

all qstringliteral uses except loadpackage() do not store strings in the static 
containers, i believe. for loadpackage calls i have removed qstringliterals in 
favour of qlatin1string


- Nick


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126148/#review88735
---


On Nov. 23, 2015, 10:19 p.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126148/
> ---
> 
> (Updated Nov. 23, 2015, 10:19 p.m.)
> 
> 
> Review request for Plasma, Marco Martin and Martin Klapetek.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> also contains fix for clazy warning regarding iterating over temporary 
> roleNames() object
> 
> note that for QRegExp constructor and for QIcon::fromTheme() i used 
> QLatin1String because QStringLiteral is known to produce crashes-on-exit if 
> the code is inside a library
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 812dc96 
>   src/declarativeimports/core/datamodel.cpp 1c83bc5 
>   src/declarativeimports/core/framesvgitem.cpp 9aab228 
>   src/declarativeimports/core/iconitem.cpp f32b3d1 
>   src/plasma/applet.cpp 89c 
>   src/plasma/containment.cpp 1d166f2 
>   src/plasma/framesvg.cpp f9379b0 
>   src/plasma/pluginloader.cpp bfa5dee 
>   src/plasma/private/applet_p.cpp e935d52 
>   src/plasma/private/containment_p.cpp 3836772 
>   src/plasma/private/theme_p.cpp 7bf81d4 
>   src/plasma/scripting/scriptengine.cpp 5b2cf84 
>   src/plasma/svg.cpp 20cb31f 
>   src/plasma/theme.cpp c49ad4c 
>   src/plasmaquick/appletquickitem.cpp efe8611 
>   src/plasmaquick/shellpluginloader.cpp 2824760 
> 
> Diff: https://git.reviewboard.kde.org/r/126148/diff/
> 
> 
> Testing
> ---
> 
> compiles fine
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126148: [plasma-framework] qstring optimizations

2015-11-23 Thread Kai Uwe Broulik 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126148/#review88737
---



src/declarativeimports/core/datamodel.cpp (line 58)


Why not constBegin()/constEnd()?



src/declarativeimports/core/iconitem.cpp (line 125)


QLatin1Char?



src/plasmaquick/shellpluginloader.cpp (line 41)


we usually don't have more than one statement per line


- Kai Uwe Broulik


On Nov. 23, 2015, 8:36 nachm., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126148/
> ---
> 
> (Updated Nov. 23, 2015, 8:36 nachm.)
> 
> 
> Review request for Plasma, Marco Martin and Martin Klapetek.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> also contains fix for clazy warning regarding iterating over temporary 
> roleNames() object
> 
> note that for QRegExp constructor and for QIcon::fromTheme() i used 
> QLatin1String because QStringLiteral is known to produce crashes-on-exit if 
> the code is inside a library
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 812dc96 
>   src/declarativeimports/core/datamodel.cpp 1c83bc5 
>   src/declarativeimports/core/framesvgitem.cpp 9aab228 
>   src/declarativeimports/core/iconitem.cpp f32b3d1 
>   src/plasma/applet.cpp 89c 
>   src/plasma/containment.cpp 1d166f2 
>   src/plasma/framesvg.cpp f9379b0 
>   src/plasma/pluginloader.cpp bfa5dee 
>   src/plasma/private/applet_p.cpp e935d52 
>   src/plasma/private/containment_p.cpp 3836772 
>   src/plasma/private/theme_p.cpp 7bf81d4 
>   src/plasma/scripting/scriptengine.cpp 5b2cf84 
>   src/plasma/svg.cpp 20cb31f 
>   src/plasma/theme.cpp c49ad4c 
>   src/plasmaquick/appletquickitem.cpp efe8611 
>   src/plasmaquick/shellpluginloader.cpp 2824760 
> 
> Diff: https://git.reviewboard.kde.org/r/126148/diff/
> 
> 
> Testing
> ---
> 
> compiles fine
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126148: [plasma-framework] qstring optimizations

2015-11-23 Thread Nick Shaforostoff 


> On Nov. 23, 2015, 9:53 p.m., Kai Uwe Broulik wrote:
> > src/declarativeimports/core/datamodel.cpp, line 58
> > 
> >
> > Why not constBegin()/constEnd()?

because rNames is const, constBegin()/constEnd() will be called internaly 
anyways, and begin()/end() just look nicer


> On Nov. 23, 2015, 9:53 p.m., Kai Uwe Broulik wrote:
> > src/declarativeimports/core/iconitem.cpp, line 125
> > 
> >
> > QLatin1Char?

yep, i will add that in the commit or the next diff version


> On Nov. 23, 2015, 9:53 p.m., Kai Uwe Broulik wrote:
> > src/plasmaquick/shellpluginloader.cpp, line 41
> > 
> >
> > we usually don't have more than one statement per line

ok, i will remove that change


- Nick


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126148/#review88737
---


On Nov. 23, 2015, 10:19 p.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126148/
> ---
> 
> (Updated Nov. 23, 2015, 10:19 p.m.)
> 
> 
> Review request for Plasma, Marco Martin and Martin Klapetek.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> also contains fix for clazy warning regarding iterating over temporary 
> roleNames() object
> 
> note that for QRegExp constructor and for QIcon::fromTheme() i used 
> QLatin1String because QStringLiteral is known to produce crashes-on-exit if 
> the code is inside a library
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 812dc96 
>   src/declarativeimports/core/datamodel.cpp 1c83bc5 
>   src/declarativeimports/core/framesvgitem.cpp 9aab228 
>   src/declarativeimports/core/iconitem.cpp f32b3d1 
>   src/plasma/applet.cpp 89c 
>   src/plasma/containment.cpp 1d166f2 
>   src/plasma/framesvg.cpp f9379b0 
>   src/plasma/pluginloader.cpp bfa5dee 
>   src/plasma/private/applet_p.cpp e935d52 
>   src/plasma/private/containment_p.cpp 3836772 
>   src/plasma/private/theme_p.cpp 7bf81d4 
>   src/plasma/scripting/scriptengine.cpp 5b2cf84 
>   src/plasma/svg.cpp 20cb31f 
>   src/plasma/theme.cpp c49ad4c 
>   src/plasmaquick/appletquickitem.cpp efe8611 
>   src/plasmaquick/shellpluginloader.cpp 2824760 
> 
> Diff: https://git.reviewboard.kde.org/r/126148/diff/
> 
> 
> Testing
> ---
> 
> compiles fine
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 126148: [plasma-framework] qstring optimizations

2015-11-23 Thread Aleix Pol Gonzalez 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126148/#review88735
---



CMakeLists.txt (line 122)


Why do you drop FAST_CONCATENATION?


Regarding QStringLiteral vs QLatin1String for libraries: I read that thread, 
but then most of QStringLiterals here sould be dropped as well if that was the 
case. I don't think it makes sense to make a special case of QIcon::fromTheme.

- Aleix Pol Gonzalez


On Nov. 23, 2015, 9:36 p.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126148/
> ---
> 
> (Updated Nov. 23, 2015, 9:36 p.m.)
> 
> 
> Review request for Plasma, Marco Martin and Martin Klapetek.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> ---
> 
> also contains fix for clazy warning regarding iterating over temporary 
> roleNames() object
> 
> note that for QRegExp constructor and for QIcon::fromTheme() i used 
> QLatin1String because QStringLiteral is known to produce crashes-on-exit if 
> the code is inside a library
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 812dc96 
>   src/declarativeimports/core/datamodel.cpp 1c83bc5 
>   src/declarativeimports/core/framesvgitem.cpp 9aab228 
>   src/declarativeimports/core/iconitem.cpp f32b3d1 
>   src/plasma/applet.cpp 89c 
>   src/plasma/containment.cpp 1d166f2 
>   src/plasma/framesvg.cpp f9379b0 
>   src/plasma/pluginloader.cpp bfa5dee 
>   src/plasma/private/applet_p.cpp e935d52 
>   src/plasma/private/containment_p.cpp 3836772 
>   src/plasma/private/theme_p.cpp 7bf81d4 
>   src/plasma/scripting/scriptengine.cpp 5b2cf84 
>   src/plasma/svg.cpp 20cb31f 
>   src/plasma/theme.cpp c49ad4c 
>   src/plasmaquick/appletquickitem.cpp efe8611 
>   src/plasmaquick/shellpluginloader.cpp 2824760 
> 
> Diff: https://git.reviewboard.kde.org/r/126148/diff/
> 
> 
> Testing
> ---
> 
> compiles fine
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Review Request 126148: [plasma-framework] qstring optimizations

2015-11-23 Thread Nick Shaforostoff 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126148/
---

Review request for Plasma, Marco Martin and Martin Klapetek.


Repository: plasma-framework


Description
---

also contains fix for clazy warning regarding iterating over temporary 
roleNames() object

note that for QRegExp constructor and for QIcon::fromTheme() i used 
QLatin1String because QStringLiteral is known to produce crashes-on-exit if the 
code is inside a library


Diffs
-

  CMakeLists.txt 812dc96 
  src/declarativeimports/core/datamodel.cpp 1c83bc5 
  src/declarativeimports/core/framesvgitem.cpp 9aab228 
  src/declarativeimports/core/iconitem.cpp f32b3d1 
  src/plasma/applet.cpp 89c 
  src/plasma/containment.cpp 1d166f2 
  src/plasma/framesvg.cpp f9379b0 
  src/plasma/pluginloader.cpp bfa5dee 
  src/plasma/private/applet_p.cpp e935d52 
  src/plasma/private/containment_p.cpp 3836772 
  src/plasma/private/theme_p.cpp 7bf81d4 
  src/plasma/scripting/scriptengine.cpp 5b2cf84 
  src/plasma/svg.cpp 20cb31f 
  src/plasma/theme.cpp c49ad4c 
  src/plasmaquick/appletquickitem.cpp efe8611 
  src/plasmaquick/shellpluginloader.cpp 2824760 

Diff: https://git.reviewboard.kde.org/r/126148/diff/


Testing
---

compiles fine


Thanks,

Nick Shaforostoff

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel