> On May 30, 2016, 11:34 a.m., Hugo Pereira Da Costa wrote: > > Ship It! > > Hugo Pereira Da Costa wrote: > err. Wait ... > There are rendering issues here once the patch is applied. > See http://wstaw.org/m/2016/05/30/plasma-desktopY12228.png > (left is "before", right is "after"). > So something seems wrong with the background gradient. > I'll investigate a bit ... > > Hugo Pereira Da Costa wrote: > Hi again, > So, thinking more about it, and actually answering the questions raised > in the review: > > - do we track public API for this part of Oxygen? Does anything in a > different library or application link to this? > No we don't this is supposed to be an "internal" (as in private) library, > used only by oxygen style and decoration. No ABI/API guarantee. > > - QColor: can we change the return type. I would say yes, (from reference > to value), and return to using QCache > - Tilesets: I would be inclined to changing the return type here too, > using values, and assuming the copy constructor does not cost much, based on > the implicit-shareness nature of pixmaps, and keep a built-in QCache here > (which should be more efficient than the (otherwise nice) FIFO. > > Now I understand that this is a lot of going back and forth, and a job I > should rather do myself, as the maintainer of the code. > So I would propose to "postpone" this RR for now, and for me to locally > - take the QPixmap change from this patch > - implement something similar for QColor and TileSet > - test. > Then if I manage to do that in a reasonable time (e.g. this week), drop > the review and commit my change instead (with proper credits where due). > Otherwise (because of me being too busy with other stuff), just commit this > review (once the problem mentionned above is fixed, though I could not > investigate yet). > > What do you think ? > > (also: I need to sanitize this run-time changing of the cache use and > max-cost) > > Michael Pyne wrote: > Hugo, > > That all sounds fair. And if it's all too much difficulty, then we might > be able to lean on the hinted-at-but-not-quite-documented QCache behavior > that ::insert() only fails if the item being inserted has a cost higher than > maxCost. In that case I could ignore the Coverity issues (since Coverity > can't statically prove that items are always < maxCost) and then drop the RR > (and perhaps ask Qt to document more stringently that guarantee for > QCache::insert). > > Still though, I think the QColor return type change would make sense even > independent of this RR.
Hi again Michael, So, I have a working implementation here that - keeps using QCache wherever possible, passing QColor and TileSet as values rather than as refs (in a way that is similar to the change you did for QPixmaps) - uses your FIFOCache for the implementation of Oxygen::Cache, which is a "cache of caches", and thus cannot possibly use values. Seems to work (though I would like to test a bit more), and should fix all the issues with Coverty. Since I have blatently copied your code for the home-made FiFOCache, i have added you as a copyright holder for oxygenhelper.h is that ok with you ? Should i put your name in other places too ? Best, Hugo - Hugo ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127866/#review96029 ----------------------------------------------------------- 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 > >
