xtensor-stack / xtensor

C++ tensors with broadcasting and lazy computing
BSD 3-Clause "New" or "Revised" License
3.33k stars 398 forks source link

Make final classes "final" #745

Closed JohanMabille closed 6 years ago

JohanMabille commented 6 years ago

So people are not tempted to inherit from these classes in their extensions of xtensor. Note that this would be a backward incompatible change.

ukoethe commented 6 years ago

Which classes are considered final? xarray and xtensor?

SylvainCorlay commented 6 years ago

Yes, and others. Basically the derived type of the CRTP pattern.

SylvainCorlay commented 6 years ago

Another way to put it: any xexpression type not parameterized with a derived type. Views, generators, etc...

ukoethe commented 6 years ago

If I understand this definition correctly, xvigra::view_nd (a subclass of xview_semantic) should be final, and xvigra::array_nd should not be derivable from it. However, I just now successfully did precisely this by specializing xt::detail::is_xexpression_impl appropriately (see #741).

So I tend to think that subclassing xtensor may have legitimate uses and should not be made impossible.

SylvainCorlay commented 6 years ago

In the current logic, I guess that the most logical thing to do would be for xvigra::view_nd to still be a CRTP base, used for both your view and array, then a have concrete final classes for both.

SylvainCorlay commented 6 years ago

You may be interested in the xmaterialize patter that we used for xwidgets.

Widget classes with names prefixed by x are not meant to be directly instantiated and are CRTP bases. For example, xslider is the top-most CRTP base of slider.

Widget classes with names that are not prefixed by x can be instantiated, but are final, i.e. they cannot be inherited from.

In fact, these final classes are typedefs on a special template parameterized by its base. For example, we have:

using button = xmaterialize<xbutton>;

Similarly, for template widget types, we also make use of the xmaterialize class, which

template <class T>
using slider = xmaterialize<xslider, T>;

The xmaterialize class only implements the final inheritance closing the CRTP, together with the RAII logic, which is to be done at the top-most inheritance level, so that widget creation messages are sent after all the bases have been initialized.

ukoethe commented 6 years ago

I'm happy with my design as it stands (array_nd derived from view_nd derived from xview_semantic) and don't understand what could be gained by making classes final and changing the hierarchy. I'm aiming for the semantic "an array is its own view", as expressed by my inheritance, and don't want arrays and views to be siblings derived from a common base. Since the design works as intended (as of today :smile:), it can't be entirely wrong.

Of course, this is only indirectly related to the question if xtensor should be final. But when there might be use cases where inheriting from xtensor makes sense, the established C++ policy has always been to not prevent it. C++ deliberately gives programmers the freedom to shoot themselves in the feet.

ukoethe commented 6 years ago

I googled for "Why make your C++ class final?", and the answers are either skeptical about the concept in general, or refer to very specific use cases that don't apply here. Unless every inheritance from xtensor is necessarily a bug, there is no justification for making it final, IMHO.

SylvainCorlay commented 6 years ago

The final keyword is very relevant in the CRTP pattern because most operations return the derived type template argument.

For example, in xsemantic, operator+= returns derived_type&, which is probably the view_nd and not your array_nd on which you may have called the function.

This is true for most operators, as well as the derived_cast operator which should return the concrete type.

Hence the need for the final keyword.

ukoethe commented 6 years ago

returns derived_type&, which is probably the view_nd

Indeed, but this is desired, because the parent view_nd & represents the array_nd in the xexpression framework -- xexpressions never see the array directly, and shouldn't.

SylvainCorlay commented 6 years ago

Indeed, but this is desired, because the parent view_nd & represents the array_nd in the xexpression framework -- xexpressions never see the array directly, and shouldn't.

Gotcha. Then it is very specific to your framework... Our move to final classes should not impact your inheritance from our CRTP bases.

ukoethe commented 6 years ago

Here is a use case for inheriting from xtensor: Suppose someone wants to be able to construct an xtensor from a specific class in their software. Then they have two possibilities: They can write a subclass of xtensor providing a constructor taking this class, or write a factory function. If xtensor is final, they no longer have a choice -- the first possibility is blocked. Why do you want to take this decision away from the people who are concerned?

ukoethe commented 6 years ago

Our move to final classes should not impact your inheritance from our CRTP bases.

I know. I'm just doing my best to give advice.

SylvainCorlay commented 6 years ago

I know. I'm just doing my best to give advice.

very appreciated and for all the bug reports as well.

Suppose someone wants to be able to construct an xtensor from a specific class in their software....

Note that we allow people to create they own typedefs from xarray_container<> and xtensor_container<>...

ukoethe commented 6 years ago

For example, in xsemantic, operator+= returns derived_type&, which is probably the view_nd and not your array_nd on which you may have called the function.

Yes, but if this is a problem, one can trivially overload operator+= in the derived class to call the base's operator and then return *this. I'm fighting for the freedom of your users :smile:

ukoethe commented 6 years ago

very appreciated and for all the bug reports as well.

Thanks!

JohanMabille commented 6 years ago

Closing this since it would have more constraints than benefits.