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

copilot-c99: Can C99 generator declare stream functions as static? #507

Open
simondlevy opened this issue Mar 20, 2024 · 10 comments
Open

copilot-c99: Can C99 generator declare stream functions as static? #507

simondlevy opened this issue Mar 20, 2024 · 10 comments
Assignees

Comments

@simondlevy
Copy link

C code output by Copilot declares variables static but functions global; e.g.:

static size_t s3_idx = (0);

float s0_get(size_t x) {
  return (s0)[((s0_idx) + (x)) % (1)];
}

Global declaration of functions causes duplicate-name conflicts when linking together multiple C sources output by Copilot. Issue could probably be handled by a script, but I'm wondering why the local functions are being declared without static to begin with.

@ivanperez-keera
Copy link
Member

That's a good point. I see no reason to make them public and I agree that it would be better to make functions static.

@ivanperez-keera ivanperez-keera changed the title Can C99 generator declare stream functions as static? copilot-c99: Can C99 generator declare stream functions as static? Mar 21, 2024
@ivanperez-keera
Copy link
Member

@fdedden Did you have a chance to look into this? Do you think this can be done without negative impact?

@fdedden fdedden self-assigned this Apr 19, 2024
@fdedden
Copy link
Member

fdedden commented Apr 19, 2024

Dear Simon, thank you for the suggestion. Yes, it is indeed better to have these functions defined as static. It forces the so called 'storage duration' to be as long as the execution of the program (which is what we want here). As you mentioned, adding static to global functions keeps the function private to the execution unit (similar to a file in this case) as well.

The addition of the keyword has already been implemented in January for the 3.18 release of Copilot (see 6779d03) to comply with the MISRA C coding standard. It should suffice for you to upgrade to at least the 3.18 release.

Please let me know if you have any more questions.

Reference to the C99 standard, section 6.2.2 and 6.2.4: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf.

@ivanperez-keera
Copy link
Member

The addition of the keyword has already been implemented in January for the 3.18

I think I only added it for variables, not functions, didn't I?

@fdedden
Copy link
Member

fdedden commented Apr 22, 2024

The addition of the keyword has already been implemented in January for the 3.18

I think I only added it for variables, not functions, didn't I?

No? The commit I mentioned (6779d03) adds static to the generator and accessor functions, and omits the step function (which is what we want).

The variables have been defined static since I wrote the first version of the C99 backend.

Let me also ping @simondlevy, since I forgot to do that in my initial reply.

@ivanperez-keera
Copy link
Member

You're right. I completely forgot! 😮

@ivanperez-keera
Copy link
Member

@simondlevy have you been able to try with the latest version?

@simondlevy
Copy link
Author

I bought a new computer recently and installed Copilot on it following the instructions as usual. When I tried the heater example, the compiled C header still did make the declarations static.

@ivanperez-keera
Copy link
Member

ivanperez-keera commented May 31, 2024

But were functions static or not? That was the issue, wasn't it?

@fdedden
Copy link
Member

fdedden commented Jun 17, 2024

@simondlevy Just to confirm: your issue has been solved, right?

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

No branches or pull requests

3 participants