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

+1 from me - looks much better without the while loops indeed :-)

Do you know what the maximum cache limit that will be set with FIFOCache::setMaxCost is? I'm asking because FIFOCache::find is linear in the cache size. It might be a problem if this is potentially unlimited, but if it's never much more than the default value of 256, then it probabaly doesn't matter much, because the runtime cost of re-generating the cached values is probably much larger.

FIFOCache looks like a simple solution to the problems that Coverity found with QCache (at least if the max cost is bounded) - nice!

- Frank Reininghaus

On May 22nd, 2016, 4:20 a.m. UTC, Michael Pyne wrote:

Review request for kde-workspace and Hugo Pereira Da Costa.

By Michael Pyne.

Updated May 22, 2016, 4:20 a.m.

Repository: oxygen

Description

This should mostly complete the QCache fixes I kicked off in a previous RR, 127837. Hugo noted there were many other similar usages, and boy he wasn't kidding! ;) The long story short is that these usages can theoretically cause use-after-free behavior (which can lead to crashes and even undefined behavior if the compiler ever gets smart).
NOTE It is -much- easier to review if you download the diff to your git repository for oxygen and then run "git diff -b" to ignore whitespace changes, particularly for the QPixmap changes.
For QPixmaps we return values instead of pointers, so we simply make a separate copy to be cached when we do insert. For QColor we return references to values so we must return pointers, and those have to be owned by a QCache to avoid memleaks. So I added a helper function to loop until the cache accepts the new entry. TileSets are a similar concern, except those have manual loops since I was uncertain about whether TileSet's copy constructor was the best idea or not.
This fixes a ton of Coverity issues (59717 - 259733, 259739, 259742 - 259752, 1336154, 1336155) and might be associated with Qt bug 38142 and KDE bug 219055 (which doesn't actually appear to be a dupe of a different bug to me...).

Testing

Compiled without warnings, installed and ran oxygen-demo5 -style oxygen. Used the GUI Benchmark feature to automatically cycle through all the listed features -- no crashes or obvious rendering errors.

Diffs

kdecoration/oxygendecohelper.cpp (aa75eca)
kstyle/oxygenstyle.cpp (e428606)
kstyle/oxygenstylehelper.h (9510a60)
kstyle/oxygenstylehelper.cpp (612ba37)
liboxygen/oxygenhelper.h (a6453a0)
liboxygen/oxygenhelper.cpp (4843604)
liboxygen/oxygenshadowcache.cpp (907e586)

View Diff