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.

Sign up for GitHub

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

Jump to bottom

Fix a few issues with the C generator (part 1 version 2) #14434

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

Fix a few issues with the C generator (part 1 version 2) #14434

wants to merge 5 commits into from

Conversation

eafer
Copy link

@eafer eafer commented Jan 11, 2023 •
edited
Loading

This is a second version of the pull request from #14388. The differences with the first version are:

  • this version uses the existing implementation of object_t, as requested by @ityuhui
  • the Config.cmake.in file was cleaned up (I think it showed up in the first place because I forgot to rebuild before running the script)
  • returnTypeIsEnum was abandoned and replaced with returnProperty, following the instructions by @wing328
  • the fourth patch from Fix a few issues with the C generator #14379 is also included now, because it got fixed

These patches fix a few compilation errors for this api's generated code, but they are not enough by themselves to get it to build. More patches will be submitted later.

@eafer
C: add a template for an empty any_type.h header
a194e14 
Some generated C apis fail to build because the source files get
'#include "any_type.h"' lines, but no such header gets generated. As a
simple fix, add a new template for an empty file with that name. This is
enough to fix the problem for us, because all the generic type stuff is
handled by object_t.
@eafer
C: fix enums
5a4a398 
I'm guessing that enums have not been used much with the C generator
before, because they always seem to produce code that doesn't build, or
that tries to free them after use. This patch fixes all the problems
we've encountered so far, except for those that need checking the return
type. I'll come back to that later.
@eafer
C: fix confusion of 'classname'/'classFilename'
a0fa76d
@eafer
C: fix issues with returned enums
523e5de 
Currently, the C templates never check if a function returns an enum
inside mustache, so when that happens the generated code has broken
return types and doesn't build. I originally tried to fix this by
extending CodegenOperation to implement a 'returnTypeIsEnum' check, but
William Cheng suggested[1] that I use the existing 'returnProperty'
instead:

  OpenAPITools#14379 (comment)

So do that.
@eafer
C: update the samples
7c46bc8 
As required for a pull request, run the generate-samples.sh script and
commit the changes.
@eafer
Copy link
Author

eafer commented Jan 11, 2023

@zhemant @michelealbano

@eafer eafer mentioned this pull request Jan 11, 2023
Open
@bookerdj bookerdj mentioned this pull request Apr 20, 2023
Open
6 tasks
@wing328
Copy link
Member

wing328 commented May 11, 2023

@ityuhui what do you think is the best way to proceed with this PR?

What about breaking it down into 4 smaller PRs to review and merge?

@ityuhui
Copy link
Contributor

ityuhui commented May 12, 2023

@wing328

Since it's been a long time since my last review, I will go through this PR recently to check if it can be merged or should break down into smaller PR.

@ityuhui
Copy link
Contributor

ityuhui commented May 15, 2023

I'm just wondering why this a0fa76d should change.

@wing328 Are these 2 commits 523e5de and 5a4a398 about returnProperty reviewed by you ?

I'm find with the rest of this PR.

@eafer
Copy link
Author

eafer commented May 15, 2023

Hi! Thank you for looking into this again.

I'm just wondering why this a0fa76d should change.

Because that's how *_parseFromJSON() and *_convertToJSON() are actually called by the rest of the generated code. For instance in the api-body.mustache template you have the following line:

        elementToReturn = {{{returnBaseType}}}_parseFromJSON({{classname}}localVarJSON);

For our instance_state model, this generates:

        elementToReturn = instance_state_parseFromJSON(ArmAPIlocalVarJSON);

But the *_parseFromJSON() function gets declared as:

arm_api_instance_state__e instance_state_instance_state_parseFromJSON(cJSON *instance_stateJSON);

So the build will not go well, with warnings such as this one:

warning: implicit declaration of function ‘instance_state_parseFromJSON’; did you mean instance_return_parseFromJSON’? [-Wimplicit-function-declaration]
 4045 |         elementToReturn = instance_state_parseFromJSON(ArmAPIlocalVarJSON);
      |                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                           instance_return_parseFromJSON

@ityuhui
Copy link
Contributor

ityuhui commented May 17, 2023

Thank you @eafer for the clarifacation.

I'm OK with this PR.

@wing328 If you agree the commits 523e5de and 5a4a398 about returnProperty, I think this PR can merge now.

@eafer
Copy link
Author

eafer commented May 23, 2023

Hey, is there anything more I can do here? Do you want me to walk you through those two patches?

@ityuhui
Copy link
Contributor

ityuhui commented May 24, 2023

I'm OK with this PR.

Just wait for @wing328 to review and merge.

@eafer
Copy link
Author

eafer commented May 31, 2023

Hey @wing328, is there anything more I can do to help here?

@ityuhui
Copy link
Contributor

ityuhui commented Jun 15, 2023

Hi @wing328

I think this PR can merge now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ityuhui ityuhui ityuhui approved these changes

Assignees
No one assigned
Labels
Client: C Issue: Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eafer @wing328 @ityuhui