Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Propose complete handling of records in external functions. #3198

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

HansOlsson
Copy link
Collaborator

Work in progress.

@DagBruck DagBruck marked this pull request as ready for review June 7, 2022 14:12
Copy link
Collaborator

@DagBruck DagBruck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that will work.

@HansOlsson HansOlsson mentioned this pull request Jun 10, 2022
@HansOlsson
Copy link
Collaborator Author

This has now been test-implemented in Dymola.

@henrikt-ma
Copy link
Collaborator

This is an interesting alternative to the void pointer solution. To make it match the fact that the void pointer approach has no restrictions when it comes to the names of record members, I suggest the following changes:

  • Rename the annotation CStructName to just CName.
  • Allow CName also for the members of a record. Make it required whenever the Modelica identifier isn't a valid C identifier.

Could you also elaborate a bit on the need for allowing Include on the record – why it wouldn't be sufficient to rely on the Include of the external function?

@henrikt-ma
Copy link
Collaborator

Could you also elaborate a bit on the need for allowing Include on the record – why it wouldn't be sufficient to rely on the Include of the external function?

I can try to answer this one myself: Whenever Include isn't present on the record, the tool is responsible for providing the record definition? Would it mean that the prototype for the external function making use of the struct should not come with a definition of the struct in this case?

Wouldn't it be better to say that the use of records always requires that the external function has an Include annotation that defines the struct, meaning that the tool should never provide a definition?

@HansOlsson
Copy link
Collaborator Author

HansOlsson commented Jun 12, 2022

This is an interesting alternative to the void pointer solution. To make it match the fact that the void pointer approach has no restrictions when it comes to the names of record members, I suggest the following changes:

  • Rename the annotation CStructName to just CName.
  • Allow CName also for the members of a record. Make it required whenever the Modelica identifier isn't a valid C identifier.

The first would be ok, although the 2nd reason below indicates that it may be more complicated.

I would prefer to wait with the second - for two reasons:

  • Currently we don't have any uses for it yet. It might be that it should be generalized further (e.g., specify a more complete mapping to other C-types).
  • Having the same annotation on elements and classes is somewhat problematic, since a missed (or added) semi-colon can give unexpected results. This one of those tiny quirks that really upset users. Currently we don't have any such ambiguity and we now detect that mistake in Dymola and 3D Experience platform. That would be impossible if the same annotation is valid on both.

@HansOlsson
Copy link
Collaborator Author

Could you also elaborate a bit on the need for allowing Include on the record – why it wouldn't be sufficient to rely on the Include of the external function?

I can try to answer this one myself: Whenever Include isn't present on the record, the tool is responsible for providing the record definition? Would it mean that the prototype for the external function making use of the struct should not come with a definition of the struct in this case?

Wouldn't it be better to say that the use of records always requires that the external function has an Include annotation that defines the struct, meaning that the tool should never provide a definition?

The problem is that it would break compatibility with any existing code. Currently the function could have a struct with another name and in most implementations it would just work. (Especially if there is no Include.)

However, I realize that one major improvement would be to have a better example.
Normally I would expect
annotation(Include="#include \"myFunc.h\""); - both for function and record.

@HansOlsson HansOlsson added this to the Phone 2022-3 milestone Jun 12, 2022
@henrikt-ma
Copy link
Collaborator

The problem is that it would break compatibility with any existing code. Currently the function could have a struct with another name and in most implementations it would just work. (Especially if there is no Include.)

Honestly, this doesn't sound like existing code worth defending. Why would one want to do it that way? If needed, couldn't we look for alternative migration paths for such legacy code?

@henrikt-ma
Copy link
Collaborator

henrikt-ma commented Jun 13, 2022

  • Having the same annotation on elements and classes is somewhat problematic, since a missed (or added) semi-colon can give unexpected results. This one of those tiny quirks that really upset users. Currently we don't have any such ambiguity and we now detect that mistake in Dymola and 3D Experience platform. That would be impossible if the same annotation is valid on both.

This doesn't seem like a real problem to me. While a semicolon could do all the difference, the problem is eliminated when using a GUI to set up the record, and the consequences of getting it wrong when manually editing the annotations seem easy enough to sort out.

It would just feel like an incomplete design alternative to the void pointer approach if one couldn't handle record members of any name.

@HansOlsson
Copy link
Collaborator Author

Design meeting:

Another option would be to rely on the external function include to also include the struct-definitions, but we would still need CStructName. In that case the CStructName indicates that the record definition will be available through the functions' include-annotation.

This means that the CStructName is only used if the external function is called. (Note that Wolfram System Modeler generates multiple source files.)

@HansOlsson
Copy link
Collaborator Author

It would be good to have this resolved, as we currently allow using records for external functions - and it sort of works, but relies on ABI-portability that compiler's generally warn for.

Revisiting I think that CStructName is clearer than CName. The reason is that in C (but not C++) structs form a separate name-space ( https://en.cppreference.com/w/c/language/name_space ), that's why the example has:

CStructName="MyComp_R"
...
struct MyComp_R {double x;int y;}
void MyComp_foo(const struct MyComp_R*r, struct MyComp_R2*r2);

In C++ struct on the last line is optional, but not in C.

That's a bit annoying in C and some use typedefs and other tricks to hide this (e.g, typedef struct MyComp_R MyComp_R;), that's why being clear that is the name of the struct seems like a good idea. If we want to support typedefs etc an alternative could be: CName="struct MyComp_R" (the rest of the code is unchanged).

Relying on the external function-include to provide the struct-definition seems to create odd dependencies:

  • We have one record definition giving the name of the record in C.
  • But we can have multiple functions taking that as inputs - some with include, some without, and the multiple includes can also give different definitions (bad - see also Restrictions and more examples for the Include annotation #3452 ) In order to have consistency the simplest solution is to have a common include-file, and then it seems trivial to also use it for the record.

If some really prefer that style we could document that an empty include-string for the record implies that it will be defined by the corresponding external function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants