Re: Review Request: Four simple patches for kdecore

[Rolf Eike Beer]

Dawit Alemayehu wrote:
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5775/#review8543
> ---
> 
> 
> I am personally not conveinced the need for changing from usleep to
> nanosleep in this particular instance. IOW, usleep is sufficient in this
> case to warrant a change regardless of when that system call was made
> available...
> 
> 
> /trunk/KDE/kdelibs/kdecore/util/kshareddatacache.cpp
> 
> 
> Hmm... why do unnecessary calculations in fast loops ? Surely
> "1<<21*10" can be hard coded with a #define.
> 
> 
> 
> /trunk/KDE/kdelibs/kdecore/util/kshareddatacache.cpp
> 
> 
> And of course if speed is an issue, which seemed to be the case since a
> left shift arithmetic was done above, this can be changed to a equivalent
> left shift operation:
> 
> usecSleepTime.tv_nsec = usecSleepTime.tv_nsec << 1;

No, please don't do that. That advise is outdated some years. This doesn't get 
any benefit at all since every compiler not completely outdated will generate 
the same code from "a * 2" and "a << 1". They will also do precalculations of 
constants. So this would also make the code unreadable.

See e.g. http://dl.fefe.de/optimizer-isec.pdf for more information on compiler 
optimizations.

Eike


signature.asc
Description: This is a digitally signed message part.

Re: Review Request: Four simple patches for kdecore

[Dawit Alemayehu]

---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5775/#review8543
---


I am personally not conveinced the need for changing from usleep to nanosleep 
in this particular instance. IOW, usleep is sufficient in this case to warrant 
a change regardless of when that system call was made available...


/trunk/KDE/kdelibs/kdecore/util/kshareddatacache.cpp


Hmm... why do unnecessary calculations in fast loops ? Surely "1<<21*10" 
can be hard coded with a #define.



/trunk/KDE/kdelibs/kdecore/util/kshareddatacache.cpp


And of course if speed is an issue, which seemed to be the case since a 
left shift arithmetic was done above, this can be changed to a equivalent left 
shift operation:

usecSleepTime.tv_nsec = usecSleepTime.tv_nsec << 1;


- Dawit


On 2010-11-07 14:17:29, Jaime Torres wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5775/
> ---
> 
> (Updated 2010-11-07 14:17:29)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> ---
> 
> As I do not know if that kind of patches are allowed in the freeze period, I 
> ask for them together. I'll submit them individually.
> 
> 1. ktimezone.   Include a comment with the real use of refCount.
> 2. klocale_kde. From 469 queries to paths.end() to 1 (from 0.01% to 0% in 
> callgrind)
> 3. netsupp. Remove a memory leak.
> 4. ksharedDataCache. Change the obsolete usleep (since 2001 or before) to 
> nanosleep.
> 
> 
> Diffs
> -
> 
>   /trunk/KDE/kdelibs/kdecore/date/ktimezone.cpp 1193582 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale_kde.cpp 1193582 
>   /trunk/KDE/kdelibs/kdecore/network/netsupp.cpp 1193582 
>   /trunk/KDE/kdelibs/kdecore/util/kshareddatacache.cpp 1193582 
> 
> Diff: http://svn.reviewboard.kde.org/r/5775/diff
> 
> 
> Testing
> ---
> 
> No regressions in the unit tests.
> Working with them.
> 
> 
> Thanks,
> 
> Jaime
> 
>

Re: Review Request: Four simple patches for kdecore

[Albert Astals Cid]

> On 2010-11-07 02:06:16, Dawit Alemayehu wrote:
> > /trunk/KDE/kdelibs/kdecore/network/netsupp.cpp, line 419
> > 
> >
> > Actually, the better fix here would simply be to move the code block 
> > that allocates and checks q for NULL below the "if (h == NULL)" block.
> 
> Jaime Torres wrote:
> @Albert: What I really wanted to say:
> 
> const_iterator QList::begin () const
> This is an overloaded function.
>

Sorry, but I don't understand what you mean with this comment.


- Albert


---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5775/#review8527
---


On 2010-11-07 14:17:29, Jaime Torres wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5775/
> ---
> 
> (Updated 2010-11-07 14:17:29)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> ---
> 
> As I do not know if that kind of patches are allowed in the freeze period, I 
> ask for them together. I'll submit them individually.
> 
> 1. ktimezone.   Include a comment with the real use of refCount.
> 2. klocale_kde. From 469 queries to paths.end() to 1 (from 0.01% to 0% in 
> callgrind)
> 3. netsupp. Remove a memory leak.
> 4. ksharedDataCache. Change the obsolete usleep (since 2001 or before) to 
> nanosleep.
> 
> 
> Diffs
> -
> 
>   /trunk/KDE/kdelibs/kdecore/date/ktimezone.cpp 1193582 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale_kde.cpp 1193582 
>   /trunk/KDE/kdelibs/kdecore/network/netsupp.cpp 1193582 
>   /trunk/KDE/kdelibs/kdecore/util/kshareddatacache.cpp 1193582 
> 
> Diff: http://svn.reviewboard.kde.org/r/5775/diff
> 
> 
> Testing
> ---
> 
> No regressions in the unit tests.
> Working with them.
> 
> 
> Thanks,
> 
> Jaime
> 
>

Re: Review Request: Four simple patches for kdecore

[Jaime Torres]

---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5775/
---

(Updated 2010-11-07 14:17:29.120532)


Review request for kdelibs.


Changes
---

Changed the comment. 


Summary
---

As I do not know if that kind of patches are allowed in the freeze period, I 
ask for them together. I'll submit them individually.

1. ktimezone.   Include a comment with the real use of refCount.
2. klocale_kde. From 469 queries to paths.end() to 1 (from 0.01% to 0% in 
callgrind)
3. netsupp. Remove a memory leak.
4. ksharedDataCache. Change the obsolete usleep (since 2001 or before) to 
nanosleep.


Diffs (updated)
-

  /trunk/KDE/kdelibs/kdecore/date/ktimezone.cpp 1193582 
  /trunk/KDE/kdelibs/kdecore/localization/klocale_kde.cpp 1193582 
  /trunk/KDE/kdelibs/kdecore/network/netsupp.cpp 1193582 
  /trunk/KDE/kdelibs/kdecore/util/kshareddatacache.cpp 1193582 

Diff: http://svn.reviewboard.kde.org/r/5775/diff


Testing
---

No regressions in the unit tests.
Working with them.


Thanks,

Jaime

Re: Review Request: Four simple patches for kdecore

[David Jarvie]

---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5775/#review8535
---



/trunk/KDE/kdelibs/kdecore/date/ktimezone.cpp


"use" -> "using".
(Yes, that was a typo in my earlier comment.)



/trunk/KDE/kdelibs/kdecore/date/ktimezone.cpp


I'd prefer to see the explanation about operator=() (starting from 
"Changing the") moved into the operator=() method itself.


- David


On 2010-11-07 12:54:32, Jaime Torres wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5775/
> ---
> 
> (Updated 2010-11-07 12:54:32)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> ---
> 
> As I do not know if that kind of patches are allowed in the freeze period, I 
> ask for them together. I'll submit them individually.
> 
> 1. ktimezone.   Include a comment with the real use of refCount.
> 2. klocale_kde. From 469 queries to paths.end() to 1 (from 0.01% to 0% in 
> callgrind)
> 3. netsupp. Remove a memory leak.
> 4. ksharedDataCache. Change the obsolete usleep (since 2001 or before) to 
> nanosleep.
> 
> 
> Diffs
> -
> 
>   /trunk/KDE/kdelibs/kdecore/date/ktimezone.cpp 1193582 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale_kde.cpp 1193582 
>   /trunk/KDE/kdelibs/kdecore/network/netsupp.cpp 1193582 
>   /trunk/KDE/kdelibs/kdecore/util/kshareddatacache.cpp 1193582 
> 
> Diff: http://svn.reviewboard.kde.org/r/5775/diff
> 
> 
> Testing
> ---
> 
> No regressions in the unit tests.
> Working with them.
> 
> 
> Thanks,
> 
> Jaime
> 
>

Re: Review Request: Four simple patches for kdecore

[Jaime Torres]

> On 2010-11-07 02:06:16, Dawit Alemayehu wrote:
> > /trunk/KDE/kdelibs/kdecore/network/netsupp.cpp, line 419
> > 
> >
> > Actually, the better fix here would simply be to move the code block 
> > that allocates and checks q for NULL below the "if (h == NULL)" block.

@Albert: What I really wanted to say:

const_iterator QList::begin () const
This is an overloaded function.


- Jaime


---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5775/#review8527
---


On 2010-11-07 12:54:32, Jaime Torres wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5775/
> ---
> 
> (Updated 2010-11-07 12:54:32)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> ---
> 
> As I do not know if that kind of patches are allowed in the freeze period, I 
> ask for them together. I'll submit them individually.
> 
> 1. ktimezone.   Include a comment with the real use of refCount.
> 2. klocale_kde. From 469 queries to paths.end() to 1 (from 0.01% to 0% in 
> callgrind)
> 3. netsupp. Remove a memory leak.
> 4. ksharedDataCache. Change the obsolete usleep (since 2001 or before) to 
> nanosleep.
> 
> 
> Diffs
> -
> 
>   /trunk/KDE/kdelibs/kdecore/date/ktimezone.cpp 1193582 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale_kde.cpp 1193582 
>   /trunk/KDE/kdelibs/kdecore/network/netsupp.cpp 1193582 
>   /trunk/KDE/kdelibs/kdecore/util/kshareddatacache.cpp 1193582 
> 
> Diff: http://svn.reviewboard.kde.org/r/5775/diff
> 
> 
> Testing
> ---
> 
> No regressions in the unit tests.
> Working with them.
> 
> 
> Thanks,
> 
> Jaime
> 
>

Re: Review Request: Four simple patches for kdecore

[Albert Astals Cid]

> On 2010-11-07 02:06:16, Dawit Alemayehu wrote:
> > /trunk/KDE/kdelibs/kdecore/localization/klocale_kde.cpp, line 2699
> > 
> >
> > The iterators here should be changed to constBegin() and constEnd(). 
> > Actually, there are many places in kdelibs that could benefit from such 
> > change as well... Without including the cases of non-const iterators, I get 
> > 185 hits when i execute the command below in kdelibs:
> > 
> > find -name "*.cpp" -exec grep --color -Hn "for" {} \; | grep "constEnd" 
> > | wc -l
> > 
> >

FWIW i disagree with changing perfectly valid ::begin to ::constBegin (that is 
when the container is already const)


- Albert


---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5775/#review8527
---


On 2010-11-06 22:35:06, Jaime Torres wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5775/
> ---
> 
> (Updated 2010-11-06 22:35:06)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> ---
> 
> As I do not know if that kind of patches are allowed in the freeze period, I 
> ask for them together. I'll submit them individually.
> 
> 1. ktimezone.   Include a comment with the real use of refCount.
> 2. klocale_kde. From 469 queries to paths.end() to 1 (from 0.01% to 0% in 
> callgrind)
> 3. netsupp. Remove a memory leak.
> 4. ksharedDataCache. Change the obsolete usleep (since 2001 or before) to 
> nanosleep.
> 
> 
> Diffs
> -
> 
>   /trunk/KDE/kdelibs/kdecore/date/ktimezone.cpp 1193582 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale_kde.cpp 1193582 
>   /trunk/KDE/kdelibs/kdecore/network/netsupp.cpp 1193582 
>   /trunk/KDE/kdelibs/kdecore/util/kshareddatacache.cpp 1193582 
> 
> Diff: http://svn.reviewboard.kde.org/r/5775/diff
> 
> 
> Testing
> ---
> 
> No regressions in the unit tests.
> Working with them.
> 
> 
> Thanks,
> 
> Jaime
> 
>

Re: Review Request: Four simple patches for kdecore

[Jaime Torres]

---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5775/
---

(Updated 2010-11-07 12:54:32.253284)


Review request for kdelibs.


Changes
---

Modified with Dawit improvements.
Next patch (if allowed), modify 185 for loops.

@Albert
QList provides both STL-style iterators and Java-style iterators. The STL-style 
iterators are more low-level and more cumbersome to use; on the other hand, 
they are slightly faster and, for developers who already know STL, have the 
advantage of familiarity.
http://doc.qt.nokia.com/4.7/qlist-const-iterator.html


Summary
---

As I do not know if that kind of patches are allowed in the freeze period, I 
ask for them together. I'll submit them individually.

1. ktimezone.   Include a comment with the real use of refCount.
2. klocale_kde. From 469 queries to paths.end() to 1 (from 0.01% to 0% in 
callgrind)
3. netsupp. Remove a memory leak.
4. ksharedDataCache. Change the obsolete usleep (since 2001 or before) to 
nanosleep.


Diffs (updated)
-

  /trunk/KDE/kdelibs/kdecore/date/ktimezone.cpp 1193582 
  /trunk/KDE/kdelibs/kdecore/localization/klocale_kde.cpp 1193582 
  /trunk/KDE/kdelibs/kdecore/network/netsupp.cpp 1193582 
  /trunk/KDE/kdelibs/kdecore/util/kshareddatacache.cpp 1193582 

Diff: http://svn.reviewboard.kde.org/r/5775/diff


Testing
---

No regressions in the unit tests.
Working with them.


Thanks,

Jaime

Re: Review Request: Four simple patches for kdecore

[Dawit Alemayehu]

---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5775/#review8527
---



/trunk/KDE/kdelibs/kdecore/localization/klocale_kde.cpp


The iterators here should be changed to constBegin() and constEnd(). 
Actually, there are many places in kdelibs that could benefit from such change 
as well... Without including the cases of non-const iterators, I get 185 hits 
when i execute the command below in kdelibs:

find -name "*.cpp" -exec grep --color -Hn "for" {} \; | grep "constEnd" | 
wc -l





/trunk/KDE/kdelibs/kdecore/network/netsupp.cpp


Actually, the better fix here would simply be to move the code block that 
allocates and checks q for NULL below the "if (h == NULL)" block.


- Dawit


On 2010-11-06 22:35:06, Jaime Torres wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5775/
> ---
> 
> (Updated 2010-11-06 22:35:06)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> ---
> 
> As I do not know if that kind of patches are allowed in the freeze period, I 
> ask for them together. I'll submit them individually.
> 
> 1. ktimezone.   Include a comment with the real use of refCount.
> 2. klocale_kde. From 469 queries to paths.end() to 1 (from 0.01% to 0% in 
> callgrind)
> 3. netsupp. Remove a memory leak.
> 4. ksharedDataCache. Change the obsolete usleep (since 2001 or before) to 
> nanosleep.
> 
> 
> Diffs
> -
> 
>   /trunk/KDE/kdelibs/kdecore/date/ktimezone.cpp 1193582 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale_kde.cpp 1193582 
>   /trunk/KDE/kdelibs/kdecore/network/netsupp.cpp 1193582 
>   /trunk/KDE/kdelibs/kdecore/util/kshareddatacache.cpp 1193582 
> 
> Diff: http://svn.reviewboard.kde.org/r/5775/diff
> 
> 
> Testing
> ---
> 
> No regressions in the unit tests.
> Working with them.
> 
> 
> Thanks,
> 
> Jaime
> 
>

Re: Review Request: Four simple patches for kdecore

[Jaime Torres]

---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5775/
---

(Updated 2010-11-06 22:35:06.086790)


Review request for kdelibs.


Changes
---

Sorry for not looking more carefully. I think the reference counter was done by 
QSharedDataPointer.
Instead, include your comment. I know it is trivial code for most of you, but 
not for everybody. :-(


Summary (updated)
---

As I do not know if that kind of patches are allowed in the freeze period, I 
ask for them together. I'll submit them individually.

1. ktimezone.   Include a comment with the real use of refCount.
2. klocale_kde. From 469 queries to paths.end() to 1 (from 0.01% to 0% in 
callgrind)
3. netsupp. Remove a memory leak.
4. ksharedDataCache. Change the obsolete usleep (since 2001 or before) to 
nanosleep.


Diffs (updated)
-

  /trunk/KDE/kdelibs/kdecore/date/ktimezone.cpp 1193582 
  /trunk/KDE/kdelibs/kdecore/localization/klocale_kde.cpp 1193582 
  /trunk/KDE/kdelibs/kdecore/network/netsupp.cpp 1193582 
  /trunk/KDE/kdelibs/kdecore/util/kshareddatacache.cpp 1193582 

Diff: http://svn.reviewboard.kde.org/r/5775/diff


Testing
---

No regressions in the unit tests.
Working with them.


Thanks,

Jaime

Re: Review Request: Four simple patches for kdecore

[David Jarvie]

---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5775/#review8523
---



/trunk/KDE/kdelibs/kdecore/date/ktimezone.cpp


The patch is wrong. 'refCount' holds the number of KTimeZoneBackend 
instances use the KTimeZonePrivate instance as a d-pointer. Changing the 
contents of a KTimeZonePrivate instance by means of operator=() doesn't affect 
how many references to it are held, so refCount must remain unchanged.


- David


On 2010-11-06 13:01:55, Jaime Torres wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5775/
> ---
> 
> (Updated 2010-11-06 13:01:55)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> ---
> 
> As I do not know if that kind of patches are allowed in the freeze period, I 
> ask for them together. I'll submit them individually.
> 
> 1. ktimezone.   Include the initialization of refcount in the = operator.
> 2. klocale_kde. From 469 queries to paths.end() to 1 (from 0.01% to 0% in 
> callgrind)
> 3. netsupp. Remove a memory leak.
> 4. ksharedDataCache. Change the obsolete usleep (since 2001 or before) to 
> nanosleep.
> 
> 
> Diffs
> -
> 
>   /trunk/KDE/kdelibs/kdecore/date/ktimezone.cpp 1193582 
>   /trunk/KDE/kdelibs/kdecore/localization/klocale_kde.cpp 1193582 
>   /trunk/KDE/kdelibs/kdecore/network/netsupp.cpp 1193582 
>   /trunk/KDE/kdelibs/kdecore/util/kshareddatacache.cpp 1193582 
> 
> Diff: http://svn.reviewboard.kde.org/r/5775/diff
> 
> 
> Testing
> ---
> 
> No regressions in the unit tests.
> Working with them.
> 
> 
> Thanks,
> 
> Jaime
> 
>