Inheriting from QVector<T> or std::vector<T> is a code smell and should
be avoided if possible.
Details
- Reviewers
- None
- Group Reviewers
KWin
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
utils.h | ||
---|---|---|
237 | Is it okay to use "qvector_cast" name? |
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? |
utils.h | ||
---|---|---|
244 | Good catch. |
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.
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.
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.
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. |
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. |
They don't need virtual destructors, inheritance stl containers does not do anything with destructors.
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
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
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). |
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.
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. |
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.
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.
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.
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.