Review Request for KDecoration

[Milian Wolff]

On Tuesday 04 November 2014 11:53:55 Thomas Lübking wrote:
> On Dienstag, 4. November 2014 10:20:18 CEST, Martin Gräßlin wrote:
> >> DecorationSettings
> >> onAllDesktopsAvailableChanged
> >> -> remove the "on" prefix
> > 
> > No, maybe the name is bad, but if the on is removed the meaning
> > changes. It's
> > whether one can put DecoratedClients "on all desktops" (at lest two
> > virtual
> > desktops). I'll think about a better name.
> 
> _____________
> 
> >> Generally: Ceterum censeo QList esse delendam
> >> 
> > :-D
> > :
> >> -> I suggest you remove QList and use QVector by default everywhere.
> 
> Reasoning?
> QVector operates on adjacent memory - it's very fast if you've a "create
> once, read many" structure of "reasonable" size, but when the size varies a
> lot and/or glib has trouble to allocate the amount of adjacent memory,
> things will go south.
> 
> Now, this pretty much is the case for the QList's used in this contex (so
> replacing them with QVector is certainly an improvement), but I'd be very
> careful about extrapolating your personal Cannae here ;-)

The reasoning is benchmarking I and others did in the past. In most cases, 
QVector was far superior to QList. See e.g.:

http://marcmutz.wordpress.com/2011/08/15/effective-qt-understand-the-qt-containers/

This led us to the point, where we argue that QVector should be used by 
default. You are right that one can conceive situations where QList will be 
better performance wise than a QVector. But most of the time it is not. And in 
nearly all situations, one can then come up with an even better solution that 
does not use QList either. But for the common case, a QList may be just as 
fast as a QVector (but often it is not). Thus: use QVector by default.

Also see e.g. the following. I know, QList is not a linked list, but it is 
similarly cache-unfriendly:

https://www.youtube.com/watch?v=YQs6IC-vgmo

And then there are tons of talks out there about why std::vector is far better 
than most alternatives (including hash maps or the like), in *many* 
situations. This also applies to QVector.

https://www.youtube.com/watch?v=rX0ItVEVjHc
https://www.youtube.com/watch?v=fHNmRkzxHWs
http://channel9.msdn.com/Events/Build/2013/4-329
http://channel9.msdn.com/Events/Build/2014/4-587

Combined, this is why I want people to use QVector by default and QList only 
if the profiler tells them so. In many cases, this will mean using less memory 
and being faster.

> > No, the point I wanted to make by using a QPointer is that the API doesn't
> > guarantee that the returned pointer stays valid. Kind of I want to enforce
> > correct usage (note this are plugins loaded into KWin, I don't
> > want a plugin
> > to crash KWin by accessing a dangling pointer. Better save in the API than
> > sorry).
> 
> I'd rather stress "notice that the return value can be nullptr and change
> during the lifetime of the DecoratedClient".
> 
> The user can still do
> Decoration *m_deco = m_client->decoration(); // fails at some point
> 
> Overmore, the guarded pointer may turn nullptr, but m_client->decoration()
> return a valid pointer again, right (deco changed)?
> 
> So the strategy you've to sell is to NOT store the pointer locally - and
> QPointer does actually the opposite, does it?

I agree with Thomas. Using QPointer does not give you any guarantee about 
correct usage. If you want safety, use shared pointers. A QPointer can still 
be invalid and thus you'll need to check it everywhere you use it.

> >> ::findBridge
> > 
> > could you please explain why or point me to a web resource describing it?
> 
> Make it invisible. Any local helper function, not (supposed to be) used
> outside the source file probably should be tagged "static".

http://stackoverflow.com/questions/1358400/what-is-external-linkage-and-internal-linkage-in-c

Bye
-- 
Milian Wolff
mail at milianw.de
http://milianw.de