-
- Notifications
You must be signed in to change notification settings - Fork 7.3k
Update c curl generator to support float #21103
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
Conversation
| 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? |
| Also, would you mind adding some tests for these issues to the |
dc5639c to ba044a6 Compare …nAPITools#21092)" This reverts commit ba044a6.
…nAPITools#21092)" This reverts commit f99c5b0.
…s#21092) - Convert float, double and long to string - Generate samples
| 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. |
| Thanks. I can see a new |
…s#21092) - Add missing yaml example file
| Sorry. I commited the missing yaml file. |
| Thanks. Why did you set |
…s#21092) - Transfer float and double in scientific notation with resp. 7 and 16 decimals - Adapt string size to number of required characters
| 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)). |
What's the advantage of this over just calling snprintf() twice? My concern is that the long float path will almost never get tested.
Ah yes, that makes more sense. |
| By the way, what's this change about? Are you using the returned value anywhere? |
…s#21092) - Fix unused variable. - Fix snprintf string
| 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 = ". |
…ols#21092) - Generating samples
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).
Thanks. |
…ols#21092) - Always allocate the exact string size
| Thank you for making the changes. This looks good to me now @wing328 |
| 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. |
| Sorry I missed it. Will get it reviewed tomorrow. cc @zhemant (2018/11) @ityuhui (2019/12) @michelealbano (2020/03) @eafer (2024/12) |
| 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. |
Fix for issue #21092
PR checklist
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.
master(upcoming7.x.0minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks)@eafer