thliebig / openEMS

openEMS is a free and open-source electromagnetic field solver using the EC-FDTD method.
http://openEMS.de
GNU General Public License v3.0
413 stars 146 forks source link

Eliminate Code Duplication in Engine_Extension classes using a Helper Macro #139

Open biergaizi opened 3 months ago

biergaizi commented 3 months ago

In openEMS, all extensions are subclasses from the abstract Engine_Extension to implement features like Engine_Extension::Apply2Voltages(). When an Engine_Extension is created, it accepts a pointer to Engine. Simulation data is taken from the Engine using virtual functions like Engine::GetVolt().

Because virtual functions prevent the compiler from inlining the code, in order to select the non-virtual functions, each extension repeats the same simulation code repeated many times using non-virtual subclass engine functions in a big if/else check, e.g. 2 different Apply2Voltages(), one calls Engine::GetVolt(), another calls Engine_sse::GetVolt(). This led to code duplication and poor readability.

I found it's extremely difficult to do any development within these Extensions due to the code duplication problem. I've already tried 2 solutions:

  1. Convert all Engine_Extension classes to a template Engine_Extension<typename Engine>
  2. Convert the Engine_Extension class into a static Curiously Recurring Template Pattern (CRTP)

while these solutions are cleaner, but I found the change required is too disruptive. What we need is a stop-gap solution for now, thus, I decide to add a new helper macro called ENG_DISPATCH. Now within each extension, features are first written as macros like:

Apply2VoltagesImpl<Engine>(Engine*)

which are used internally by:

Apply2Voltages()

by calling:

ENG_DISPATCH(Apply2VoltagesImpl)
// or ENG_THREAD_DISPATCH(Apply2VoltagesImpl, threadId)

This is not pretty but it significantly reduces code duplication. Using this macro, it allows me to remove 917 lines of code and would significantly simplify future programming, which will be implemented later this year.

In the long run, the proper solution should probably be converting all Extensions classes to templates. But that is left for the future.

biergaizi commented 2 months ago

Don't merge yet. Just found a name-collision problem.

thliebig commented 2 months ago

Well that works fine, I did not have time to review yet anyways ;)

biergaizi commented 2 months ago

The mentioned name collision problem has now been fix.