Warnings in .bnd C code

[casella]

Please run the PowerGrids.Examples.IEEE14bus.IEEE14busStaticNetwork in OMEdit. The following warning is generated during C compilation:

IEEE14busStaticNetwork_08bnd.c:19:157: warning: format specifies type 'long' but the argument has type 'modelica_integer' (aka 'long long') [-Wformat]
    infoStreamPrint(LOG_INIT_V, 0, "updated start value: %s(start=%ld)", data->modelData->integerVarsData[4].info /* GEN8.AVR.limiterWithLag.state */.name, (modelica_integer) (data->localData[0]->integerVars[4] /* GEN8.AVR.limiterWithLag.state DISCRETE */));
                                                                  ~~~                                                                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                                  %lld

I guess that doesn't matter much unless really long integers are used, but I think we should have corrected C code nevertheless.

@mahge I think this was added when you managed to fix the thorny issue with start values bound to parameter expressions, would you mind having a look?

[mahge]

This happens because OpenModelica's modelica_integer is long on Linux and long long on Windows. So the warning happens only on Windows now. If we change it to %lld, there will be no warning on Windows but we will have one on Linux.

The most logical thing to do would be to find the code-generation code responsible for this and fix it to generate the correct format specifier for the current OS. Unfortunately, that information about the OS, is not available right at the point where the code-generator generates this code. Making it available there will require some amount of changes because of how Susan works (you would have to pass it through a chain of functions). I remember seeing some global variables being used for some features. Maybe that can help here as well.

The second, also logical but a bit ugly for the eyes, solution is to not directly print the format specifiers, but to use pre-processor variables in their place. These preprocessor variables can then be defined according to the OS. These approach is already being used in a few places in the runtime system. With this, we change the generated code from:

infoStreamPrint(LOG_INIT_V, 0, "updated start value: %s(start=%ld)", data->modelData->integerVarsData[4].info ....);

To

infoStreamPrint(LOG_INIT_V, 0, "updated start value: %s(start=" OMC_INT_FORMAT ")", data->modelData->integerVarsData[4].info ...);

Although it might be a bit confusing at first sight, this works fine as well.

[casella]

This happens because OpenModelica's modelica_integer is long on Linux and long long on Windows.

Ouch. Right.

The most logical thing to do would be to find the code-generation code responsible for this and fix it to generate the correct format specifier for the current OS. Unfortunately, that information about the OS, is not available right at the point where the code-generator generates this code. Making it available there will require some amount of changes because of how Susan works (you would have to pass it through a chain of functions). I remember seeing some global variables being used for some features. Maybe that can help here as well.

The second, also logical but a bit ugly for the eyes, solution is to not directly print the format specifiers, but to use pre-processor variables in their place. These preprocessor variables can then be defined according to the OS. These approach is already being used in a few places in the runtime system. With this, we change the generated code from:

I am definitetly in favour of using macros. Seems the most logical thing to do, given the state of the affairs. I hope it's not too much work to fix it.

infoStreamPrint(LOG_INIT_V, 0, "updated start value: %s(start=%ld)", data->modelData->integerVarsData[4].info ....);

To

infoStreamPrint(LOG_INIT_V, 0, "updated start value: %s(start=" OMC_INT_FORMAT ")", data->modelData->integerVarsData[4].info ...);

Shouldn't it be

infoStreamPrint(LOG_INIT_V, 0, "updated start value: %s(start=OMC_INT_FORMAT)", data->modelData->integerVarsData[4].info ...);

?

[mahge]

Shouldn't it be

infoStreamPrint(LOG_INIT_V, 0, "updated start value: %s(start=OMC_INT_FORMAT)", data->modelData->integerVarsData[4].info 

Unfortunately no. It might be confusing at first sight 🙂 and that is why I hesitated before going ahead and adding it.

The C-pre-processor (CPP) macro replacement does not work inside literal strings. It ignores string literals completely. So the string (simplified a bit)

"updated start value: start=OMC_INT_FORMAT)"

would not be changed by the CPP even when OMC_INT_FORMAT is a macro.

To get around that, we have to split the string into three parts:

   "updated start value: start=" OMC_INT_FORMAT ")"
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~  ~~~~~~~~~~~~~  ~~~
//               1                     2         3

which will be pre-processed to

   "updated start value: start=" "%ld" ")"
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~  ~~~~  ~~~
//               1                2     3

and then concatenated to one string by the C compiler.

[casella]

The C-pre-processor (CPP) macro replacement does not work inside literal strings.

Aha, I was not aware of that. Applied witchcraft 101 😄

Do you have a PR ready with that?

[mahge]

This should be fixed with #10738. It is a little confusing but it turns out the other alternative is equally, if not more, confusing (See c8c6d33).

Modifying the codegen to pass around the information about the current OS would have been better. My plan was to do this in a variable called context that we pass around to most codegen functions. Unfortunately, some of new contexts created are created in places where we do not have the whole SimCode info. Leading to more confusing implementations. It would have been nice to use the context variable for more things like this (passing around more info during codegen, for example I use them to pass around cref prefixes for creation of record constructor functions). However, for this specific issue it turned out to be too much a of an overkill because the simulation part of the codegen does not pass around the context fully. Plus it creates new contexts randomly when needed.

Maybe something else pushes us to fix all this soon. Until then the macro based fix should be okay for fixing the warnings in this issue.

[casella]

I confirm the .bnd file warnings are gone. There are others left, see #10701. @mahge could you please fix them in the same way?

Thanks!