Re: Review Request 127215: simplify code, reduce pointer dereferences

2016-03-02 Thread Nick Shaforostoff 

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

(Updated March 2, 2016, 12:03 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Changes
---

Submitted with commit b9c8b1fadcbb742e6129b77f28336c223a18b424 by Nick 
Shaforostoff to branch master.


Repository: kservice


Description
---

Changes are mostly related to containers/iterators, but there are also few 
QString related optimizations

note that i have made a switch from QLinkedList to just QList because it 
performs better for T=
also i have changed QStringList to QSet in one place to make the code 
run faster


Diffs
-

  src/services/kservice.cpp c75bce2 
  src/services/kservicetype.cpp 2a73ccd 
  src/sycoca/kbuildmimetypefactory.cpp 340f76a 
  src/sycoca/kbuildservicefactory.cpp a74f2e8 
  src/sycoca/kbuildsycoca.cpp e99e906 
  src/sycoca/kmimeassociations.cpp 9423b27 
  src/sycoca/ksycocadict.cpp f33d01a 
  src/sycoca/ksycocafactory.cpp e410581 
  src/sycoca/ksycocautils_p.h 8db26b0 

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


Testing
---

all tests pass, except kmimeassociationstest (fakeApplicationService is NULL), 
but it fails also without my changes


Thanks,

Nick Shaforostoff

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127215: simplify code, reduce pointer dereferences

2016-03-02 Thread David Faure 

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


Ship it!




Well, the Qt bug isn't invalid, there's a bug if the documentation is 
incomplete

Thanks for the patch, looks good now.

- David Faure


On Feb. 29, 2016, 12:27 p.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127215/
> ---
> 
> (Updated Feb. 29, 2016, 12:27 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Changes are mostly related to containers/iterators, but there are also few 
> QString related optimizations
> 
> note that i have made a switch from QLinkedList to just QList because it 
> performs better for T=
> also i have changed QStringList to QSet in one place to make the 
> code run faster
> 
> 
> Diffs
> -
> 
>   src/services/kservice.cpp c75bce2 
>   src/services/kservicetype.cpp 2a73ccd 
>   src/sycoca/kbuildmimetypefactory.cpp 340f76a 
>   src/sycoca/kbuildservicefactory.cpp a74f2e8 
>   src/sycoca/kbuildsycoca.cpp e99e906 
>   src/sycoca/kmimeassociations.cpp 9423b27 
>   src/sycoca/ksycocadict.cpp f33d01a 
>   src/sycoca/ksycocafactory.cpp e410581 
>   src/sycoca/ksycocautils_p.h 8db26b0 
> 
> Diff: https://git.reviewboard.kde.org/r/127215/diff/
> 
> 
> Testing
> ---
> 
> all tests pass, except kmimeassociationstest (fakeApplicationService is 
> NULL), but it fails also without my changes
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127215: simplify code, reduce pointer dereferences

2016-03-01 Thread Nick Shaforostoff 


> On Feb. 29, 2016, 4:12 a.m., Aleix Pol Gonzalez wrote:
> > src/sycoca/ksycocadict.cpp, line 322
> > 
> >
> > Maybe it's better to leave the * there, as it makes it more clear that 
> > it's an output argument.

well it's not really an output argument it is a 'processReplacing', the code 
only modifier the content pointed by the list, not the list itself (you see 
this by const_iterators used for walking through the list), so would everybody 
agree if i change the argument to 'const KSycocaDictStringList&'?


- Nick


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


On Feb. 29, 2016, 12:27 p.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127215/
> ---
> 
> (Updated Feb. 29, 2016, 12:27 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Changes are mostly related to containers/iterators, but there are also few 
> QString related optimizations
> 
> note that i have made a switch from QLinkedList to just QList because it 
> performs better for T=
> also i have changed QStringList to QSet in one place to make the 
> code run faster
> 
> 
> Diffs
> -
> 
>   src/services/kservice.cpp c75bce2 
>   src/services/kservicetype.cpp 2a73ccd 
>   src/sycoca/kbuildmimetypefactory.cpp 340f76a 
>   src/sycoca/kbuildservicefactory.cpp a74f2e8 
>   src/sycoca/kbuildsycoca.cpp e99e906 
>   src/sycoca/kmimeassociations.cpp 9423b27 
>   src/sycoca/ksycocadict.cpp f33d01a 
>   src/sycoca/ksycocafactory.cpp e410581 
>   src/sycoca/ksycocautils_p.h 8db26b0 
> 
> Diff: https://git.reviewboard.kde.org/r/127215/diff/
> 
> 
> Testing
> ---
> 
> all tests pass, except kmimeassociationstest (fakeApplicationService is 
> NULL), but it fails also without my changes
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127215: simplify code, reduce pointer dereferences

2016-03-01 Thread Nick Shaforostoff 


> On Feb. 29, 2016, 8:14 a.m., David Faure wrote:
> > src/sycoca/kbuildmimetypefactory.cpp, line 65
> > 
> >
> > Can you keep the order of the operands? I find
> >   if (var == constant)
> > more readable than the other way around.
> > ("is this car green?", not "is green the color of this car?"). In any 
> > case that's how it is everywhere in KF5 AFAIK.
> 
> Nick Shaforostoff wrote:
> i have changed to a natural order. but the problem is that i couldn't 
> find an appropriate operator== overload taking (qstringref, qlatin1string), 
> there is only a (qlatin1string, qstringref) one. is that a qt bug that should 
> be fixed?
> 
> David Faure wrote:
> Sounds like it.

i have reported the bug, but it was closed as invalid: turns out there is such 
overload, but it is not reflected in the reference


- Nick


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


On Feb. 29, 2016, 12:27 p.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127215/
> ---
> 
> (Updated Feb. 29, 2016, 12:27 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Changes are mostly related to containers/iterators, but there are also few 
> QString related optimizations
> 
> note that i have made a switch from QLinkedList to just QList because it 
> performs better for T=
> also i have changed QStringList to QSet in one place to make the 
> code run faster
> 
> 
> Diffs
> -
> 
>   src/services/kservice.cpp c75bce2 
>   src/services/kservicetype.cpp 2a73ccd 
>   src/sycoca/kbuildmimetypefactory.cpp 340f76a 
>   src/sycoca/kbuildservicefactory.cpp a74f2e8 
>   src/sycoca/kbuildsycoca.cpp e99e906 
>   src/sycoca/kmimeassociations.cpp 9423b27 
>   src/sycoca/ksycocadict.cpp f33d01a 
>   src/sycoca/ksycocafactory.cpp e410581 
>   src/sycoca/ksycocautils_p.h 8db26b0 
> 
> Diff: https://git.reviewboard.kde.org/r/127215/diff/
> 
> 
> Testing
> ---
> 
> all tests pass, except kmimeassociationstest (fakeApplicationService is 
> NULL), but it fails also without my changes
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127215: simplify code, reduce pointer dereferences

2016-03-01 Thread David Faure 


> On Feb. 29, 2016, 12:31 p.m., Kai Uwe Broulik wrote:
> > src/sycoca/kbuildsycoca.cpp, line 143
> > 
> >
> > QVector maybe?
> 
> Nick Shaforostoff wrote:
> doesn't make a difference from performance perspective, see 
> https://marcmutz.wordpress.com/effective-qt/containers/
> but just changing qlist to qvector here increases the lib binary size by 
> 15kb (!)

maybe because QList is already much more used in this code? not sure


- David


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


On Feb. 29, 2016, 12:27 p.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127215/
> ---
> 
> (Updated Feb. 29, 2016, 12:27 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Changes are mostly related to containers/iterators, but there are also few 
> QString related optimizations
> 
> note that i have made a switch from QLinkedList to just QList because it 
> performs better for T=
> also i have changed QStringList to QSet in one place to make the 
> code run faster
> 
> 
> Diffs
> -
> 
>   src/services/kservice.cpp c75bce2 
>   src/services/kservicetype.cpp 2a73ccd 
>   src/sycoca/kbuildmimetypefactory.cpp 340f76a 
>   src/sycoca/kbuildservicefactory.cpp a74f2e8 
>   src/sycoca/kbuildsycoca.cpp e99e906 
>   src/sycoca/kmimeassociations.cpp 9423b27 
>   src/sycoca/ksycocadict.cpp f33d01a 
>   src/sycoca/ksycocafactory.cpp e410581 
>   src/sycoca/ksycocautils_p.h 8db26b0 
> 
> Diff: https://git.reviewboard.kde.org/r/127215/diff/
> 
> 
> Testing
> ---
> 
> all tests pass, except kmimeassociationstest (fakeApplicationService is 
> NULL), but it fails also without my changes
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127215: simplify code, reduce pointer dereferences

2016-03-01 Thread David Faure 


> On Feb. 29, 2016, 8:14 a.m., David Faure wrote:
> > About the unittest failure, I'm available by email to debug that, tests 
> > shouldn't fail.
> > (first thing to check will be: did a 
> > ~/.qttest/share/applications/faketextapplication.desktop get created?)
> 
> Nick Shaforostoff wrote:
> the test started to pass once i configured bash environment for 
> kdesrc-buil correctly

cool


> On Feb. 29, 2016, 8:14 a.m., David Faure wrote:
> > src/sycoca/kbuildmimetypefactory.cpp, line 65
> > 
> >
> > Can you keep the order of the operands? I find
> >   if (var == constant)
> > more readable than the other way around.
> > ("is this car green?", not "is green the color of this car?"). In any 
> > case that's how it is everywhere in KF5 AFAIK.
> 
> Nick Shaforostoff wrote:
> i have changed to a natural order. but the problem is that i couldn't 
> find an appropriate operator== overload taking (qstringref, qlatin1string), 
> there is only a (qlatin1string, qstringref) one. is that a qt bug that should 
> be fixed?

Sounds like it.


> On Feb. 29, 2016, 8:14 a.m., David Faure wrote:
> > src/sycoca/kbuildsycoca.cpp, line 209
> > 
> >
> > what is the clazy warning that encourages to write such a long line of 
> > code, rather than the two lines as it was initially?
> 
> Nick Shaforostoff wrote:
> since Q_FOREACH takes a copy anyways i thought that putting the call 
> inside it is a little improvement.
> i have reverted this part.

"copy" is a big word, it only increases a refcount.


- David


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


On Feb. 29, 2016, 12:27 p.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127215/
> ---
> 
> (Updated Feb. 29, 2016, 12:27 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Changes are mostly related to containers/iterators, but there are also few 
> QString related optimizations
> 
> note that i have made a switch from QLinkedList to just QList because it 
> performs better for T=
> also i have changed QStringList to QSet in one place to make the 
> code run faster
> 
> 
> Diffs
> -
> 
>   src/services/kservice.cpp c75bce2 
>   src/services/kservicetype.cpp 2a73ccd 
>   src/sycoca/kbuildmimetypefactory.cpp 340f76a 
>   src/sycoca/kbuildservicefactory.cpp a74f2e8 
>   src/sycoca/kbuildsycoca.cpp e99e906 
>   src/sycoca/kmimeassociations.cpp 9423b27 
>   src/sycoca/ksycocadict.cpp f33d01a 
>   src/sycoca/ksycocafactory.cpp e410581 
>   src/sycoca/ksycocautils_p.h 8db26b0 
> 
> Diff: https://git.reviewboard.kde.org/r/127215/diff/
> 
> 
> Testing
> ---
> 
> all tests pass, except kmimeassociationstest (fakeApplicationService is 
> NULL), but it fails also without my changes
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127215: simplify code, reduce pointer dereferences

2016-03-01 Thread Nick Shaforostoff 

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



so, any objections for submitting the patch?

- Nick Shaforostoff


On Feb. 29, 2016, 12:27 p.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127215/
> ---
> 
> (Updated Feb. 29, 2016, 12:27 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Changes are mostly related to containers/iterators, but there are also few 
> QString related optimizations
> 
> note that i have made a switch from QLinkedList to just QList because it 
> performs better for T=
> also i have changed QStringList to QSet in one place to make the 
> code run faster
> 
> 
> Diffs
> -
> 
>   src/services/kservice.cpp c75bce2 
>   src/services/kservicetype.cpp 2a73ccd 
>   src/sycoca/kbuildmimetypefactory.cpp 340f76a 
>   src/sycoca/kbuildservicefactory.cpp a74f2e8 
>   src/sycoca/kbuildsycoca.cpp e99e906 
>   src/sycoca/kmimeassociations.cpp 9423b27 
>   src/sycoca/ksycocadict.cpp f33d01a 
>   src/sycoca/ksycocafactory.cpp e410581 
>   src/sycoca/ksycocautils_p.h 8db26b0 
> 
> Diff: https://git.reviewboard.kde.org/r/127215/diff/
> 
> 
> Testing
> ---
> 
> all tests pass, except kmimeassociationstest (fakeApplicationService is 
> NULL), but it fails also without my changes
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127215: simplify code, reduce pointer dereferences

2016-02-29 Thread Nick Shaforostoff 


> On Feb. 29, 2016, 12:31 p.m., Kai Uwe Broulik wrote:
> > src/sycoca/kbuildservicefactory.cpp, line 167
> > 
> >
> > We can't use range-for in Frameworks, right?

in this case i benefit from Q_FOREACH taking a copy of the container


> On Feb. 29, 2016, 12:31 p.m., Kai Uwe Broulik wrote:
> > src/sycoca/kbuildsycoca.cpp, line 143
> > 
> >
> > QVector maybe?

doesn't make a difference from performance perspective, see 
https://marcmutz.wordpress.com/effective-qt/containers/
but just changing qlist to qvector here increases the lib binary size by 15kb 
(!)


> On Feb. 29, 2016, 12:31 p.m., Kai Uwe Broulik wrote:
> > src/sycoca/kbuildsycoca.cpp, lines 246-248
> > 
> >
> > Cache end?

that's too much ))
in other places caching did make sence because container was accessed via 
pointer


- Nick


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


On Feb. 29, 2016, 12:27 p.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127215/
> ---
> 
> (Updated Feb. 29, 2016, 12:27 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Changes are mostly related to containers/iterators, but there are also few 
> QString related optimizations
> 
> note that i have made a switch from QLinkedList to just QList because it 
> performs better for T=
> also i have changed QStringList to QSet in one place to make the 
> code run faster
> 
> 
> Diffs
> -
> 
>   src/services/kservice.cpp c75bce2 
>   src/services/kservicetype.cpp 2a73ccd 
>   src/sycoca/kbuildmimetypefactory.cpp 340f76a 
>   src/sycoca/kbuildservicefactory.cpp a74f2e8 
>   src/sycoca/kbuildsycoca.cpp e99e906 
>   src/sycoca/kmimeassociations.cpp 9423b27 
>   src/sycoca/ksycocadict.cpp f33d01a 
>   src/sycoca/ksycocafactory.cpp e410581 
>   src/sycoca/ksycocautils_p.h 8db26b0 
> 
> Diff: https://git.reviewboard.kde.org/r/127215/diff/
> 
> 
> Testing
> ---
> 
> all tests pass, except kmimeassociationstest (fakeApplicationService is 
> NULL), but it fails also without my changes
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127215: simplify code, reduce pointer dereferences

2016-02-29 Thread Kai Uwe Broulik 

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




src/sycoca/kbuildservicefactory.cpp (line 162)


We can't use range-for in Frameworks, right?



src/sycoca/kbuildsycoca.cpp (line 143)


QVector maybe?



src/sycoca/kbuildsycoca.cpp (lines 236 - 238)


Cache end?


- Kai Uwe Broulik


On Feb. 29, 2016, 12:27 nachm., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127215/
> ---
> 
> (Updated Feb. 29, 2016, 12:27 nachm.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Changes are mostly related to containers/iterators, but there are also few 
> QString related optimizations
> 
> note that i have made a switch from QLinkedList to just QList because it 
> performs better for T=
> also i have changed QStringList to QSet in one place to make the 
> code run faster
> 
> 
> Diffs
> -
> 
>   src/services/kservice.cpp c75bce2 
>   src/services/kservicetype.cpp 2a73ccd 
>   src/sycoca/kbuildmimetypefactory.cpp 340f76a 
>   src/sycoca/kbuildservicefactory.cpp a74f2e8 
>   src/sycoca/kbuildsycoca.cpp e99e906 
>   src/sycoca/kmimeassociations.cpp 9423b27 
>   src/sycoca/ksycocadict.cpp f33d01a 
>   src/sycoca/ksycocafactory.cpp e410581 
>   src/sycoca/ksycocautils_p.h 8db26b0 
> 
> Diff: https://git.reviewboard.kde.org/r/127215/diff/
> 
> 
> Testing
> ---
> 
> all tests pass, except kmimeassociationstest (fakeApplicationService is 
> NULL), but it fails also without my changes
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127215: simplify code, reduce pointer dereferences

2016-02-29 Thread Nick Shaforostoff 


> On Feb. 29, 2016, 4:12 a.m., Aleix Pol Gonzalez wrote:
> > src/sycoca/kbuildsycoca.cpp, line 246
> > 
> >
> > foreach like you did above?

for loops with simple body i prefer direct iterators to a copy-taking Q_FOREACH


- Nick


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


On Feb. 29, 2016, 12:27 p.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127215/
> ---
> 
> (Updated Feb. 29, 2016, 12:27 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Changes are mostly related to containers/iterators, but there are also few 
> QString related optimizations
> 
> note that i have made a switch from QLinkedList to just QList because it 
> performs better for T=
> also i have changed QStringList to QSet in one place to make the 
> code run faster
> 
> 
> Diffs
> -
> 
>   src/services/kservice.cpp c75bce2 
>   src/services/kservicetype.cpp 2a73ccd 
>   src/sycoca/kbuildmimetypefactory.cpp 340f76a 
>   src/sycoca/kbuildservicefactory.cpp a74f2e8 
>   src/sycoca/kbuildsycoca.cpp e99e906 
>   src/sycoca/kmimeassociations.cpp 9423b27 
>   src/sycoca/ksycocadict.cpp f33d01a 
>   src/sycoca/ksycocafactory.cpp e410581 
>   src/sycoca/ksycocautils_p.h 8db26b0 
> 
> Diff: https://git.reviewboard.kde.org/r/127215/diff/
> 
> 
> Testing
> ---
> 
> all tests pass, except kmimeassociationstest (fakeApplicationService is 
> NULL), but it fails also without my changes
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127215: simplify code, reduce pointer dereferences

2016-02-29 Thread Nick Shaforostoff 

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

(Updated Feb. 29, 2016, 12:27 p.m.)


Review request for KDE Frameworks.


Changes
---

followed the suggestions, and also changed contains(QStringLiteral(...)) to 
contains(QLatin1String(...))


Repository: kservice


Description
---

Changes are mostly related to containers/iterators, but there are also few 
QString related optimizations

note that i have made a switch from QLinkedList to just QList because it 
performs better for T=
also i have changed QStringList to QSet in one place to make the code 
run faster


Diffs (updated)
-

  src/services/kservice.cpp c75bce2 
  src/services/kservicetype.cpp 2a73ccd 
  src/sycoca/kbuildmimetypefactory.cpp 340f76a 
  src/sycoca/kbuildservicefactory.cpp a74f2e8 
  src/sycoca/kbuildsycoca.cpp e99e906 
  src/sycoca/kmimeassociations.cpp 9423b27 
  src/sycoca/ksycocadict.cpp f33d01a 
  src/sycoca/ksycocafactory.cpp e410581 
  src/sycoca/ksycocautils_p.h 8db26b0 

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


Testing
---

all tests pass, except kmimeassociationstest (fakeApplicationService is NULL), 
but it fails also without my changes


Thanks,

Nick Shaforostoff

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127215: simplify code, reduce pointer dereferences

2016-02-29 Thread David Faure 

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



About the unittest failure, I'm available by email to debug that, tests 
shouldn't fail.
(first thing to check will be: did a 
~/.qttest/share/applications/faketextapplication.desktop get created?)


src/sycoca/kbuildmimetypefactory.cpp (line 59)


Can you keep the order of the operands? I find
  if (var == constant)
more readable than the other way around.
("is this car green?", not "is green the color of this car?"). In any case 
that's how it is everywhere in KF5 AFAIK.



src/sycoca/kbuildservicefactory.cpp (line 85)


It took me some time to understand the comment -1 +1 = 0, since here there 
_is_ just a +1. Position after the slash -> +1. No -1 anywhere.

But then I got it - this is about the case where there is no '/', then we 
get -1 and start from 0. It's obvious to me, while the comment isn't ;)
So I'd say remove the comment, or make it more explicit ("if no slash, -1 + 
1 = 0").



src/sycoca/kbuildservicefactory.cpp (line 96)


Shouldn't this one be a QStringLiteral?



src/sycoca/kbuildservicefactory.cpp (line 210)


You could have kept the "const" in front of endserv (same below)



src/sycoca/kbuildsycoca.cpp (line 204)


what is the clazy warning that encourages to write such a long line of 
code, rather than the two lines as it was initially?



src/sycoca/ksycocadict.cpp (line 316)


I agree.


- David Faure


On Feb. 29, 2016, 12:43 a.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127215/
> ---
> 
> (Updated Feb. 29, 2016, 12:43 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Changes are mostly related to containers/iterators, but there are also few 
> QString related optimizations
> 
> note that i have made a switch from QLinkedList to just QList because it 
> performs better for T=
> also i have changed QStringList to QSet in one place to make the 
> code run faster
> 
> 
> Diffs
> -
> 
>   src/sycoca/kbuildmimetypefactory.cpp 340f76a 
>   src/sycoca/kbuildservicefactory.cpp a74f2e8 
>   src/sycoca/kbuildsycoca.cpp e99e906 
>   src/sycoca/kmimeassociations.cpp 9423b27 
>   src/sycoca/ksycocadict.cpp f33d01a 
>   src/sycoca/ksycocafactory.cpp e410581 
> 
> Diff: https://git.reviewboard.kde.org/r/127215/diff/
> 
> 
> Testing
> ---
> 
> all tests pass, except kmimeassociationstest (fakeApplicationService is 
> NULL), but it fails also without my changes
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127215: simplify code, reduce pointer dereferences

2016-02-28 Thread Aleix Pol Gonzalez 

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




src/sycoca/kbuildsycoca.cpp (line 236)


foreach like you did above?



src/sycoca/ksycocadict.cpp (line 316)


Maybe it's better to leave the * there, as it makes it more clear that it's 
an output argument.


- Aleix Pol Gonzalez


On Feb. 29, 2016, 1:43 a.m., Nick Shaforostoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127215/
> ---
> 
> (Updated Feb. 29, 2016, 1:43 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Changes are mostly related to containers/iterators, but there are also few 
> QString related optimizations
> 
> note that i have made a switch from QLinkedList to just QList because it 
> performs better for T=
> also i have changed QStringList to QSet in one place to make the 
> code run faster
> 
> 
> Diffs
> -
> 
>   src/sycoca/kbuildmimetypefactory.cpp 340f76a 
>   src/sycoca/kbuildservicefactory.cpp a74f2e8 
>   src/sycoca/kbuildsycoca.cpp e99e906 
>   src/sycoca/kmimeassociations.cpp 9423b27 
>   src/sycoca/ksycocadict.cpp f33d01a 
>   src/sycoca/ksycocafactory.cpp e410581 
> 
> Diff: https://git.reviewboard.kde.org/r/127215/diff/
> 
> 
> Testing
> ---
> 
> all tests pass, except kmimeassociationstest (fakeApplicationService is 
> NULL), but it fails also without my changes
> 
> 
> Thanks,
> 
> Nick Shaforostoff
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel