Topic: Replacing "giveInterface" with "dynamic_cast"

It seems to me that we could remove all the "giveInterface" functions and just rely on dynamic_cast instead.
A fair bit of code could be removed in the process (every giveInterface, and the Interface types), requiring much less modifications to add new interfaces.
I don't see any disadvantage, so unless there is any objections I will do this modification soon.

Re: Replacing "giveInterface" with "dynamic_cast"

Don't really know if this is good or not.
However, I have just sent patches to Borek to be added. I hope that they will be added before everything changes.

3

Re: Replacing "giveInterface" with "dynamic_cast"

Well, I can see at least one drawback: if the class is derived from specific interface, then the dynamic_cast solution will always return interface, which means that the class implements the functionality. But sometimes the class is derived from parent, that implements the interface, but the child (not yet) implements the interface, then the soluton using dynamic_cast is unable to support this. The existing approach using giveInterface method can deal with that, as the child can always overload the giveInterface.
We can discuss whether it is a good desing, when parent implements the interface and child not, but it can happen (and is the case already in oofem).

Re: Replacing "giveInterface" with "dynamic_cast"

Borek, do you happen to know which particular classes have broken implementations of the inherited interfaces?
I can't think of any fast way to find them.
If its not very difficult to support those extensions, then fixing them seems like a good idea, wether or not anything is changed with giveInterface.
And if it really is difficult to support the extension the superclass has, then that would make a pretty good case against the design.

With constantly growing code size, i think maintainability of the code is very important. There is plenty of old code in OOFEM already, and being able to remove a huge chunk is something I always strive for.
There are plenty of scenarios where things are quite likely to go wrong due to this.
E.g. If a superclass implements a new interface, which the subclass automatically supports then you would still have to go through all subclasses and manually add this.
And for newcomers, supporting an interface would be as simple as just implementing it, as opposed to adding a new interface type, overloading giveInterface (and check through all superclasses and adding those interfaces as well), followed by casting from the general "Interface" type returned.

There is also plenty of documentation around this, which I know many new developers stumple upon (i know i did  before i realized its basically just a dynamic_cast (without the cast)).

5

Re: Replacing "giveInterface" with "dynamic_cast"

I agree with some of your points, however, I still think that the ability to implement just a raw class (like an element having just plane structural capability) that is derived from existing element without the need to implement (or check) all interfaces enforced by parent, can be beneficial. We have many small student projects, where students just adapt the existing elements or material models, but they simply don't need any error estimation capability, etc, imposed by one of interfaces. They just want to get basic structural capability, however, the base class already enforces some interfaces. Then it is in my opinion better and safer to mark those as not supported, as opposed to pretending automatic interface implementation. And in principle, this allows to accept such an basic element/material into oofem (as the basic functionality is required in most cases and could represent a significant contribution). If not able to selectively implement interfaces, we could not in principle accept such an incomplete class, and that could be a pitty for us.

Re: Replacing "giveInterface" with "dynamic_cast"

The real problem there is that the new class is inheriting from an unsuitable superclass (or possible, the superclass promises more interfaces than it should).
It might be tempting to inherit from a class because you need something very similar, but inheritance should be logical, not saving you a bit of code.
And if it is a proper subclass, then its a simple "TODO" to fix that missing functionality (just don't expose it in the documentation and it should be consider W.I.P, no need to refuse it).
You can put a OOFEM_ERROR("Not implemented yet"); into your interface function, just as you would have for Element::computeMidPlaneNormal or Element::computeVolume

Just looking at a few elements (and without verifying), then I would bet that e.g. MacroLSpace, TrPlaneStrRot3d, PlaneStress2dXfem don't actually properly support all the interfaces their respective superclass inherits. And none of them actually limits the giveInterface() function anyway, i.e. they are broken (at least I can't see how MacroLSpace and Xfem can set up refined elements for the Huerta estimator). Just as "broken" as the dynamic_cast would have been, but noone has even noticed.

For inheritance diagrams;
http://www.oofem.org/resources/doc/oofe … Space.html
http://www.oofem.org/resources/doc/oofe … Rot3d.html
http://www.oofem.org/resources/doc/oofe … dXfem.html

I would rather reject patches which wouldn't support dynamic_cast by the basis that it's badly designed.
Weighing in the positive and negative, I my opinion, dynamic_cast comes out out strongly in favor.
That's really all I have to say about it so I'll drop it now.