Skip to content

Conversation

@amoweb
Copy link
Contributor

@amoweb amoweb commented Apr 17, 2025

Fix for issue #21092

PR checklist

  • Read the contribution guidelines.
  • [ x Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package || exit ./bin/generate-samples.sh ./bin/configs/*.yaml || exit ./bin/utils/export_docs_generators.sh || exit 
    (For Windows users, please run the script in Git BASH)
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@eafer

@eafer
Copy link
Contributor

eafer commented Apr 17, 2025

Hi! I just took a quick look at the new generated samples, and I see that petId for deletePet becomes a pointer. What's the point of that? The petId is not nullable, right?

@eafer
Copy link
Contributor

eafer commented Apr 17, 2025

Also, would you mind adding some tests for these issues to the petstore.yaml for C? That would make it much easier to understand what you are doing, and it would prevent other people from breaking this again in the future.

amoweb added 2 commits April 28, 2025 13:38
…s#21092) - Change function signature to float* - Change generator to convert float to string - Similar change for double and long
…s#21092) - Generate samples - Fix generator for headers and path params
@amoweb amoweb force-pushed the fix_c_curl_generator branch from dc5639c to ba044a6 Compare April 28, 2025 11:38
@amoweb
Copy link
Contributor Author

amoweb commented Apr 28, 2025

Hi! Thanks for your answer @eafer ! Considering your comments, I changed the strategy. I added an example in petstore.yaml and I no longer change the function signature.

@eafer
Copy link
Contributor

eafer commented Apr 28, 2025

Thanks. I can see a new UserAPI_testInt32Int64FloatDouble() function in the generated samples, but I'm not seeing the changes to the yaml file that made that happen. Am I missing something, or maybe you forgot to commit that?

@amoweb
Copy link
Contributor Author

amoweb commented Apr 29, 2025

Sorry. I commited the missing yaml file.

@eafer
Copy link
Contributor

eafer commented Apr 29, 2025

Thanks. Why did you set MAX_NUMBER_LENGTH_FLOAT to 32, is this an arbitrary limit? For big float numbers the output of snprintf() will be off. I think in this case it would be better to call snprintf twice, the first time to get the size of the buffer needed.

…s#21092) - Transfer float and double in scientific notation with resp. 7 and 16 decimals - Adapt string size to number of required characters
@amoweb
Copy link
Contributor Author

amoweb commented May 6, 2025

Good remark. I defined a default size and reallocate only if needed (so in most case, we only call snprintf once).

Additionally, I figured out that %f displays only 6 decimals by default which is not sufficient for double. I think %.7e for float and %.16e for double is more relevant since they roughly correspond to the float precision (log2(2**significand)).

@eafer
Copy link
Contributor

eafer commented May 6, 2025

I defined a default size and reallocate only if needed (so in most case, we only call snprintf once).

What's the advantage of this over just calling snprintf() twice? My concern is that the long float path will almost never get tested.

I think %.7e for float and %.16e for double is more relevant since they roughly correspond to the float precision

Ah yes, that makes more sense.

@eafer
Copy link
Contributor

eafer commented May 6, 2025

By the way, what's this change about? Are you using the returned value anywhere?

+ int s = snprintf(valueQuery_{{{paramName}}}, MAX_NUMBER_LENGTH, "%d", *{{{paramName}}}); 
@amoweb
Copy link
Contributor Author

amoweb commented May 7, 2025

I think it's less performant to call snprintf() twice. The initial size is sufficient for most numbers (except very big ones). I can still change it if you prefer.

I removed the useless "int s = ".

@eafer
Copy link
Contributor

eafer commented May 7, 2025

I think it's less performant to call snprintf() twice.

Sure, but this code only runs right before making a blocking network request, so I don't think it could ever be a bottleneck. I think it's better to keep it simple (unless you have any benchmarks that show otherwise).

I removed the useless "int s = ".

Thanks.

@eafer
Copy link
Contributor

eafer commented May 12, 2025

Thank you for making the changes. This looks good to me now @wing328

@amoweb
Copy link
Contributor Author

amoweb commented Oct 7, 2025

@eafer @wing328 Hello, did you had time to have a look to my PR? Thanks

@eafer
Copy link
Contributor

eafer commented Oct 7, 2025

I had already agreed to your PR, I guess @wing328 never saw this. Of course it's been a while so this may no longer apply cleanly.

@wing328
Copy link
Member

wing328 commented Oct 7, 2025

Sorry I missed it. Will get it reviewed tomorrow.

cc @zhemant (2018/11) @ityuhui (2019/12) @michelealbano (2020/03) @eafer (2024/12)

@wing328 wing328 changed the title Fix c curl generator Update c curl generator to support float Oct 8, 2025
@wing328 wing328 added this to the 7.17.0 milestone Oct 8, 2025
@wing328 wing328 merged commit 6b1b5cc into OpenAPITools:master Oct 8, 2025
4 checks passed
@wing328
Copy link
Member

wing328 commented Oct 8, 2025

Thanks for the PR, which has been merged.

Sorry for the delay as there are too many PRs for this project.

Have a good day.

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

3 participants