Review Request 127866: Oxygen: Fix QCache usage

[Hugo Pereira Da Costa]

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



Hi, 
thanks for the updated patch. No more objection.
Just wanted to mention that in principle, tileset could probably be passed around by value too: they are "mostly" only a QVector<QPixmap> (which would be implicitely cached, right ?), and a bunch of integers. So that copying them should not involve too much operations.

- Hugo Pereira Da Costa


On May 22, 2016, 4:20 a.m., Michael Pyne wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127866/
> -----------------------------------------------------------
> 
> (Updated May 22, 2016, 4:20 a.m.)
> 
> 
> Review request for kde-workspace and Hugo Pereira Da Costa.
> 
> 
> 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...).
> 
> 
> 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 
> 
> Diff: https://git.reviewboard.kde.org/r/127866/diff/
> 
> 
> 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.
> 
> 
> Thanks,
> 
> Michael Pyne
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20160530/ae9b4b13/attachment.htm>