RFC: Drop Outputs class
AbandonedPublic

Authored by zzag on Sep 12 2019, 10:20 PM.

Details

Reviewers
None
Group Reviewers
KWin
Summary

Inheriting from QVector<T> or std::vector<T> is a code smell and should
be avoided if possible.

Test Plan

Compiles.

Diff Detail

Repository
R108 KWin
Branch
yo-my-name-is-vlad-all-nights-im-fooling-with-new-stuff
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 16549
Build 16567: arc lint + arc unit
zzag created this revision.Sep 12 2019, 10:20 PM
Restricted Application added a project: KWin. · View Herald TranscriptSep 12 2019, 10:20 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Sep 12 2019, 10:20 PM
zzag added inline comments.Sep 12 2019, 10:21 PM
utils.h
237

Is it okay to use "qvector_cast" name?

alexeymin added inline comments.
utils.h
237

Looks as cool as your branch name

I don't care either way. @davidedmundson came up with the Output as child of QVector construct. So it's important what he thinks.

utils.h
244

std::copy can't be used this way?

Doesn't solve anything and now we have casts all over the place.

zzag updated this revision to Diff 65975.Sep 13 2019, 10:59 AM

Use std::copy().

zzag marked an inline comment as done.Sep 13 2019, 10:59 AM
zzag added inline comments.
utils.h
244

Good catch.

zzag marked an inline comment as done.EditedSep 13 2019, 11:04 AM

Doesn't solve anything and now we have casts all over the place.

Well, it doesn't cure cancer, that's for sure, however it fixes a code smell.

Can you provide some arguments why Ouputs must inherit from QVector<T>? C++ templates are invariant so we have to cast manually.

davidedmundson added a comment.EditedSep 13 2019, 2:35 PM

Can you provide some arguments why we must inherit from QVector<T>? C++ templates are invariant so we have to cast manually.

It doesn't inherit from QVector<T>, it inhertis from QVector<AbstractOutput*> It has a constructor from QVector<T>.

Ultimately the code is doing the same thing.
Previous code does it more implicitly, this does everything slightly more explicitly.

zzag added a comment.Sep 13 2019, 3:22 PM

I haven't received any strong arguments why we need a subclass of QVector<AbstractOutput *>. Outputs overloads constructor to implement some sort of covariance. However, that doesn't qualify as a good argument because the right solution would be to add a helper function that could perform these conversions.

In many cases, inheriting from a vector type is a no-no. You should do that only if there's no any other way around.


This change makes everything too explicit. Boo!

I personally don't consider it to be a problem. That's C++ after all.

Templates are invariant and making conversions between Collection<Base> and Collection<Derived> explicit is a good thing because this way we have less magic in the code.

In D23918#530689, @zzag wrote:

I haven't received any strong arguments why we need a subclass of QVector<AbstractOutput *>. Outputs overloads constructor to implement some sort of covariance. However, that doesn't qualify as a good argument because the right solution would be to add a helper function that could perform these conversions.

In many cases, inheriting from a vector type is a no-no. You should do that only if there's no any other way around.

You need to provide sources for such generic statements. If you would be a computer science professor of 30 years I would believe them without asking for such but we are not.


This change makes everything too explicit. Boo!

I personally don't consider it to be a problem. That's C++ after all.

Templates are invariant and making conversions between Collection<Base> and Collection<Derived> explicit is a good thing because this way we have less magic in the code.

Independent of researched facts I agree though with Vlad that the qvector_cast feels better since it's more explicit and David's QVector subclassing, while being an interesting solution, feels kind of forbidden magic in comparison.

utils.h
240

For simple data types according to Qt docs resize is faster. It's a micro-optimization, but since the outputs are queried often maybe it's worth it. I assume we could use template specialization for AbstractOutput* as To type, or just use no template at all? Although having a templated qvector_cast is nice.

zzag added a comment.Sep 14 2019, 10:18 AM

You need to provide sources for such generic statements.

Sorry to disappoint you but that's an unwritten rule in C++ community. The main reason why one should avoid inheriting qvector or std::vector has something to do with destructors. More specifically, neither one of those container types has virtual destructor. However, you could fix that with private inheritance, which will look very ugly! From design perspective if you want to transform a container you have to use an algorithm rather than inherit from the container type and add a method, i.e. don't go against STL design. It somewhat relates to composition over inheritance [1].

Let's talk about Outputs class specifically. The main problem with it is that it adds only a constructor and nothing more. In the end we have a class without anything new to offer than its base class [QVector<AbstractOutput *>]. The worst thing is that we have yet another container type to deal with. The correct solution is to use a helper function to perform conversions between QVector<Base> and QVector<Derived> rather than using new container type.

[1] https://en.wikipedia.org/wiki/Composition_over_inheritance

utils.h
240

We could use std::enable_if or std::enable_if_t to specialize pointer types, however I don't think this level of complexity worth the trouble. We want to copy primitive and complex types.

Append method costs only an if statement and an integer increment operation. We can stick with the current solution unless benchmarks show that we need to make concrete assumptions about internal implementation of qvector.

In D23918#530932, @zzag wrote:

Sorry to disappoint you but that's an unwritten rule in C++ community. The main reason why one should avoid inheriting qvector or std::vector has something to do with destructors. More specifically, neither one of those container types has virtual destructor. However, you could fix that with private inheritance, which will look very ugly!

They don't need virtual destructors, inheritance stl containers does not do anything with destructors.

zzag added a comment.Sep 14 2019, 10:51 AM

Nope, it does because vector is not truly polymorphic.

You don't need virtual destructor when you don't have at least one virtual method, since you don't have at least one that method you don't want to delete object by its pointer to base. You use it to extend its functionality since that you declare objects by derived type, also you can add virtual destructor to derived class then extend with virtual methods and keep pointer to middle one. There is nothing wrong with stl containers or any other class without virtual destructor without at a virtual method.
https://onlinegdb.com/Bk9IKrc8S

zzag added a comment.Sep 14 2019, 11:24 AM

You don't need virtual destructor when you don't have at least one virtual method, since you don't have at least one that method you don't want to delete object by its pointer to base. You use it to extend its functionality since that you declare objects by derived type, also you can add virtual destructor to derived class then extend with virtual methods and keep pointer to middle one.

Um, I would say that's really bad code unless you use private inheritance.

There is nothing wrong with stl containers or any other class without virtual destructor without at a virtual method.
https://onlinegdb.com/Bk9IKrc8S

In D23918#530952, @zzag wrote:

Um, I would say that's really bad code unless you use private inheritance.

There is nothing bad, it's perfectly valid, same as vector does not have virtual destructor and it shouldn't have.

In general I agree with @zzag - composition is better than inheritance since it decouples things.
Many modern programming languages don't even really allow inheritance any more at all.
I wonder if there isn't a solution that avoids all the casting, I cannot say that I'm a fan of the qvector_cast. I didn't really read the code though, just glanced over it.

utils.h
237

Technically Qt reserves itself the right to qsomething_functions. A name clash is unlikely, but I would have expected the function to be part of Qt when reading the code without having any background. In that sense I'd be for a kvector_cast (not really, I don't have a good name and don't like the cast much).

zzag updated this revision to Diff 66068.Sep 14 2019, 5:03 PM

Rename qvector_cast to vector_cast.

How would it be to only keep one vector of AbstractOutputs in Platform? Then there is no vector copying. A cast should in my opinion not be a memory intense operation in the first place, but just a change of how types are interpreted.
In the sub-classes the outputs could then actually be cast as needed (in a convenience function, in case it's more than one or two places). Hopefully this would also reduce code duplication. Of course it only works if there is only one platform in use at a time (I assume that is the case right now already).

Going one step further, for the public side of things, I'd prefer to use Output as name, instead of AbstractOutput. That there is a platform specific implementation should not matter for the consumer of the public API.

gladhorn added inline comments.Sep 14 2019, 5:49 PM
platform.h
411

This could even be non-virtual in my suggestion:

QVector<Output *> outputs() const { return m_outputs; }
552

QVector<Output*> m_outputs;

plugins/platforms/drm/drm_backend.h
83–85

This would simply go away, the base class version is enough.

plugins/platforms/wayland/wayland_backend.cpp
352–353

Here a cast from Output* to WaylandOutput* would be needed.

zzag updated this revision to Diff 66081.Sep 14 2019, 7:00 PM

We don't actually need to cast vectors in standalone X11 backend.

zzag added a comment.EditedSep 14 2019, 7:05 PM

How would it be to only keep one vector of AbstractOutputs in Platform? Then there is no vector copying. A cast should in my opinion not be a memory intense operation in the first place, but just a change of how types are interpreted.
In the sub-classes the outputs could then actually be cast as needed (in a convenience function, in case it's more than one or two places). Hopefully this would also reduce code duplication. Of course it only works if there is only one platform in use at a time (I assume that is the case right now already).

Hmm, that might be one way to go. Its biggest disadvantage is that we'll have to put a few static_casts here and there as well that Platform would need to have some protected methods to alter private m_outputs field.

Going one step further, for the public side of things, I'd prefer to use Output as name, instead of AbstractOutput. That there is a platform specific implementation should not matter for the consumer of the public API.

I think Roman just wanted AbstractOutput to be in line with AbstractClient. However, I agree that it's better to drop "Abstract" prefix.

zzag updated this revision to Diff 66084.Sep 14 2019, 7:18 PM

Remove unused forward declaration.

Yes, it really depends on how many casts it ends up being. Maybe with a helper function. I personally think the vector_cast is almost as bad as inheriting the vector.

In general I agree with @zzag - composition is better than inheritance since it decouples things.

The principle is not in question here. The Outputs child class provides a conversion of QVector used for output-alike classes through constructor-override. It's an implementation detail and to be honest it's a non-issue, not worth our time.

In the sub-classes the outputs could then actually be cast as needed (in a convenience function, in case it's more than one or two places). Hopefully this would also reduce code duplication. Of course it only works if there is only one platform in use at a time (I assume that is the > case right now already).

Such a change could be more interesting but it's also without large benefits right now.

Going one step further, for the public side of things, I'd prefer to use Output as name, instead of AbstractOutput. That there is a platform specific implementation should not matter for the consumer of the public API.

Agree, the private structs in the AbstractEglBackend children are called the same though making it harder to distinguish. And since we have no consumers of public API it's also without benefit. I talked to David at Akademy about the idea of splitting out Platform and Outputs into a lib independent of KWin such that on the one side we have KWayland and on the other side the platform lib allowing to write compositors in the middle of both of them. In this case renaming this class would make sense. Before that it's just a finger exercise costing us time.

zzag added a comment.Sep 14 2019, 9:39 PM

Agree, the private structs in the AbstractEglBackend children are called the same though making it harder to distinguish. And since we have no consumers of public API it's also without benefit. I talked to David at Akademy about the idea of splitting out Platform and Outputs into a lib independent of KWin such that on the one side we have KWayland and on the other side the platform lib allowing to write compositors in the middle of both of them. In this case renaming this class would make sense. Before that it's just a finger exercise costing us time.

Please don't! Putting internals of kwin into a lib means that we have to maintain API and ABI compatibility, which means we have to be extra careful with changing Platform.

zzag abandoned this revision.Sep 23 2019, 4:00 PM