modelica
Make array inputs of external C functions const (Modelica Language Specification version 3.5)
It should be possible to merge this even if the MSL was based on MLS 3.4 since the code should still be backwards compatible. The code as it is today breaks tools that pass const data.
I do not think this should be done as some tools generate the function prototypes according to MLS 3.4.
Then the best solution would be to not include the prototypes at all (like it was in MSL 3.2.3). Because it is really annoying to try to use libraries that assume MLS 3.5 in their prototypes at the same time as using models from the MSL...
It is a breaking change, which means a tool/library cannot be compliant to MLS 3.4 and 3.5 at the same time.
If a tool generates the function prototypes they need to match the actual function implementation. I believe w/o a proper fix of Mo#1726 the dilemma cannot be resolved,
If a tool generates the function prototypes they need to match the actual function implementation.
No, it doesn't. As long as the implementation does not modify the inputs, it will work fine in both cases.
I believe w/o a proper fix of Mo#1726 the dilemma cannot be resolved,
There could be other solutions for this as well. One would be that the tool tells the C-code which language standard it is using. Like -DMODELICA_LANGUAGE_VERSION=0x030500
Some compilers are strict and require consistent types for declararion and definition.
Anyway, this PR must not be merged right now.
Some compilers are strict and require consistent types for declararion and definition.
Anyway, this PR must not be merged right now.
If there is no Include annotation, there is no conflict. And the actual interface is the same and will work fine in both Modelica 3.4 and 3.5 compilers.
If a tool generates the function prototypes they need to match the actual function implementation. I believe w/o a proper fix of Mo#1726 the dilemma cannot be resolved,
But tools shall not generate prototypes if an Include-annotation is given, see last paragraph before https://specification.modelica.org/master/functions.html#argument-type-mapping. Hence, one just has to make sure that the all the MSL external functions have Include-annotations.
Some compilers are strict and require consistent types for declararion and definition.
Yes, and this is nice. It helps detecting errors, such as tools guessing prototypes that they shouldn't.
But tools shall not generate prototypes if an
Include-annotation is given, see last paragraph before https://specification.modelica.org/master/functions.html#argument-type-mapping. Hence, one just has to make sure that the all the MSL external functions haveInclude-annotations.
Yes, but then the prototype would need to be according to MLS 3.5. As it is, one of our code generators tries to use the prototype and calls it with a constant vector... But it's declared as modifying the vector.
Yes, but then the prototype would need to be according to MLS 3.5. As it is, one of our code generators tries to use the prototype and calls it with a constant vector... But it's declared as modifying the vector.
OK, then I understand, and agree that this can only be merged once we're using MLS 3.5.
OK, then I understand, and agree that this can only be merged once we're using MLS 3.5.
I think it could be merged before then because you can call the MLS 3.5 prototype with inputs corresponding to MLS 3.4. MLS 3.5 is more permissive in what inputs are allowed (no MLS 3.4 tool can pass constant vectors as input, so the opposite direction is no problem). This PR should work fine in C/C++ with -Wall -Werror when passing non-const inputs.
There are some alternatives.
- Just merge this. It's fine.
- Modify this to also remove
Includeannotation. This is also fine, and works in tools that both generate prototype and use the Include annotation at the same time (really not recommended) - Just change to MLS 3.5 in MSL master as soon as possible.
- Just merge this. It's fine.
I'd call it incorrect, but expected to work.
- Modify this to also remove
Includeannotation. This is also fine, and works in tools that both generate prototype and use the Include annotation at the same time (really not recommended)
Clang doesn't like conflicting prototypes (the non-const corresponding to the automatically generated MLS 3.4 prototype):
const.c:2:5: error: conflicting types for 'foo'
int foo(const int* x);
^
const.c:1:5: note: previous declaration is here
int foo(int* x);
^
Clang doesn't like conflicting prototypes (the non-const corresponding to the automatically generated MLS 3.4 prototype):
const.c:2:5: error: conflicting types for 'foo' int foo(const int* x); ^ const.c:1:5: note: previous declaration is here int foo(int* x); ^
That's why I said it's an option to remove the Include annotation. Then there will be no conflict as only the automatically generated would be present.
Oh yeah, we could also have
- Introduce some magic preprocessor directive available in MLS 3.6+ so the code could add const to the prototype if it knows that the tool expects more recent MLS...
That's why I said it's an option to remove the Include annotation. Then there will be no conflict as only the automatically generated would be present.
As @tbeu pointed out, a nice compiler will reject an implementation that doesn't match the prototype. My point was that a correct prototype cannot coexist at all with the incorrect prototype generated by the tool, which could cause problems if the generated prototype would end up in a compilation unit where some other external function's Include-annotation would pull in the correct prototype.
Yes, it would indeed be unfortunate to have a mix of Include or not Include annotations being used. And thinking about it, there are libraries in the wild that use libraries from the MSL in their own external "C" functions...
Blocking accidental merge. Need to wait until master is based on MLS 3.5.
This has now been changed (to 3.6 to be specific). @dietmarw can you update?
PR should be rebased first and missing functions (e.g., ModelicaStandardTables_CombiTimeTable_init3) need to be taken into account.
It would help if there was a formal decision what MSL version would now allow MLS 3.6 features so the milestone of this PR can be set accordingly.
It would help if there was a formal decision what MSL version would now allow MLS 3.6 features so the milestone of this PR can be set accordingly.
Same comment as https://github.com/modelica/ModelicaStandardLibrary/pull/3686#issuecomment-1853434631
PR should be rebased first and missing functions (e.g., ModelicaStandardTables_CombiTimeTable_init3) need to be taken into account.
Rebased and applied to more occurrences, see https://github.com/modelica/ModelicaStandardLibrary/pull/3700/commits/3fb58752d88ce711779cbe54986c824bdf3e09d9.
@sjoelund, @rfranke, @HansOlsson I need two last positive reviews to merge this into 4.1.0
Deadline: 02 Feb 2024.
Thanks!
