> 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,