Closed rfranke closed 1 year ago
@fedetft maybe you can help me with this at some point
Cool, we found another shortcoming in the Modelica specification that I was not aware of, if I understood correctly it seems to rely on the lack of type-safety of the C language when calling external functions that take records. I wonder if, in the long term, it would be possible to add to the Modelica specification a record annotation like ImplementationName="ExternalThermodynamicState" to direct the C code generation to call the struct with the proper name...
About how to fix this issue in ExternalMedia today, rather than using the preprocessor to change the type when the header is used to compile ExternalMedia itself vs code using it, wouldn't it be better to really change the function parameter to void * and in the implementation of the function just doing a
auto *typedState = reinterpret_cast<ExternalThermodynamicState*>(state);
This solution in my opinion reduces the amount of magic needed to get the code to compile, I don't like fooling the compiler by switching the type of a function parameter between translation units. Also note the #ifdef solution only works because the C++ function is declared extern "C", otherwise it would NOT work due to the functions having different name mangling, something that would cause the linker to fail.
I also disagree with the comment in https://github.com/modelica/ModelicaSpecification/issues/3154 that void * "is a really bad idea as you abandon all type-checking", since this part of the Modelica specification only works BECAUSE it relies on the C compiler not doing type checks, so once it's broken by design, who cares...
I spoke with @casella, a pull request that changes the record function types to void * should be simple (unless I'm missing something, in this case we can discuss) and would be welcome.
PR #74 implements the change. I agree that the interface should explicitly use void* and the implementation in externalmodialib.cpp
cast them to the actual structs. I wasn't aware of this clear separation between interface and implementation before. Also, I was afraid that with void
the distinction between ExternalThermodynamicState
and ExternalSaturationProperties
would be lost. But this is not the case due to consequent use of the variable names state
and sat
, respectively.
Yes, internally ExternalMedia is well designed with a BaseSolver class and there are no type safety issues, the extern C functions are just a thin layer to let Modelica call into it.
To be honest I hope that someday the Modelica specification will allow directly exposing C++ classes to Modelica models without passing through C functions and void*, being both object-oriented languages it would seem natural to do so, but no, only C and Fortran...
An external C++ interface would be cool, in particular as ever more scientific software for productive use is being written in C++, such as CoolProp. It might take some time for Modelica though.
Here is a nice example for the potential of C++ benefiting from better code optimization (at the cost of compilation time):
ThermofluidStream.Examples.EspressoMachine
on a Windows laptop with recent OpenModelica nightly:
compilation simulation
C runtime: 8s 50s
Cpp runtime: 18s 4s
That's extreme and might not be a fair comparison. At Modelica 2015 I had made a fair comparison using the identical solver through FMI for model exchange. This had resulted in a speedup of 5-10 for model evaluation with C++.
I'm not the one you have to convince about the benefits of C++, I've been using it for 15 years in various fields ranging from embedded to high-performance computing, and always enjoyed the possibility to abstract and optimize at the same time.
Thanks for the patches, the issue should be fixed.
ExternalMedia uses the two C structs
ExternalThermodynamicState
andExternalSaturationProperties
in its interface. The fileexternalmedialib.h
declares functions such as:Modelica only defines the memory mapping but not the type of records/structs in external function calls (see https://specification.modelica.org/master/functions.html#records). Accordingly, ExternalMedia passes a record
ExternalMedia.Media.BaseClasses.ExternalTwoPhaseMedium.ThermodynamicState
when the external function expectsExternalThermodynamicState
. The Modelica compiler does not know anything aboutExternalThermodynamicState
.This leads to a compilation warning with C (e.g. OpenModelica C runtime with
ExternalMedia.Test.CoolProp.Pentane_hs_state
):C++ is more strict and raises an error (e.g. OpenModelica C++ runtime):
The warning and the error disappear when using an anonymous pointer in the declaration of the external function. This appears more in line with the treatment of records in the external function interface of Modelica.
The following modification of
externalmedialib.h
keeps the struct type definitions when compiling ExternalMediaLib and changes tovoid*
when importing the interface to Modelica.Would such a change be possible? If yes, then I could prepare a pull request.