C overhead fixes #20402

imaami · 2025-01-05T19:51:33Z

Cut down on redundant strlen() churn and similar somewhat. Functionality does not change, just runtime cost.

Request: when merging would it be possible to not squash the three commits but instead just rebase? Mashing even a moderately small changeset like this into a monolithic single diff makes reading the git log significantly more taxing cognitively. You can always do git diff HEAD~3 HEAD to see a combined diff, but you can't recover the lost information after squashing.

wing328 · 2025-01-06T02:54:20Z

thanks for the PR

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

wing328 · 2025-01-06T12:52:48Z

but you can't recover the lost information after squashing.

did you try git reflog? it works for me

https://stackoverflow.com/a/16931979/677735

imaami · 2025-01-06T16:31:38Z

but you can't recover the lost information after squashing.

did you try git reflog? it works for me

https://stackoverflow.com/a/16931979/677735

Git reflog is great, but it's entirely local. No history from any squash lives on the server. Reflog on a fresh clone looks like the attached screenshot.

Also, even the local reflog entries will eventually expire and get deleted automatically.

eafer · 2025-01-06T19:54:41Z

Hey @imaami, can you see my review comment this time?

imaami · 2025-01-06T20:47:52Z

Hey @imaami, can you see my review comment this time?

I can't, I looked at the diff and conversations, nothing. Are you sure you didn't leave review comments but didn't finish the review yet?

imaami · 2025-01-06T20:52:20Z

I think review comments won't show up until the review is fully completed. Non-review comments do show immediately, and of course it's possible to also review without approving or requesting changes if the intent is just to post comments.

imaami · 2025-01-06T20:59:30Z

Rebased onto current master.

eafer · 2025-01-06T19:38:50Z

modules/openapi-generator/src/main/resources/C-libcurl/api-body.mustache

@@ -126,7 +124,7 @@ end:
    {{#pathParams}}

    // Path Params
-    long sizeOfPathParams_{{{paramName}}} = {{#pathParams}}{{#isLong}}sizeof({{paramName}})+3{{/isLong}}{{#isString}}strlen({{^isEnum}}{{paramName}}{{/isEnum}}{{#isEnum}}{{{operationId}}}_{{enumName}}_ToString({{paramName}}){{/isEnum}})+3{{/isString}}{{^-last}} + {{/-last}}{{/pathParams}} + strlen("{ {{baseName}} }");
+    long sizeOfPathParams_{{{paramName}}} = {{#pathParams}}{{#isLong}}sizeof({{paramName}})+3{{/isLong}}{{#isString}}strlen({{^isEnum}}{{paramName}}{{/isEnum}}{{#isEnum}}{{{operationId}}}_{{enumName}}_ToString({{paramName}}){{/isEnum}})+3{{/isString}}{{^-last}} + {{/-last}}{{/pathParams}} + sizeof "{ {{baseName}} }" - 1;


I think the style here usually includes brackets for sizeof, you can see it before in the same line.

I think the style here usually includes brackets for sizeof, you can see it before in the same line.

It's true that sizeof is often used with parentheses, but the situation is similar to return: neither are functions. sizeof is a unary operator and the parens are only required when the operand is a type. So both of the sizeof expressions below are valid and evaluate to the size of int:

int *p1 = malloc(sizeof *p1); int *p2 = malloc(sizeof(int));

Operator precedence for sizeof is higher than addition and substraction (as well as multiplication and division), so it doesn't need surrounding parens, either.

If this is a style thing I can change it of course.

https://en.cppreference.com/w/c/language/sizeof

I don't care particularly either way, all I'm pointing out is that the generator currently uses the brackets. I won't argue further if you prefer it this way.

eafer · 2025-01-06T22:27:34Z

I think review comments won't show up until the review is fully completed.

Ah, I get it now. I'm always lost with github.

wing328 · 2025-01-07T08:13:50Z

Also, even the local reflog entries will eventually expire and get deleted automatically.

I see. Thanks for pointing it out.

For merged PRs, the git message shows the PR number, e.g.

commit cba756ffa6b8a8ee15d456699fe36143a2f08f72 (HEAD -> master, origin/master, origin/HEAD)
Author: Andriy Slobodyanyk <[email protected]>
Date:   Mon Jan 6 16:08:01 2025 +0400

    Adds @Nullable annotation to Spring Boot generator (#20345)

    * Adds @Nullable annotation to Spring Boot generator

    * issue-14427: [REQ][spring] Null-Safety annotations
    * issue-17382: [REQ] spring generator add Nullable annotations

    Motivations:
    * Have Spring Boot generator client properly annotated for nullability to be able to check code using them with tools like NullAway
    * As it is related to Spring then the `org.springframework.lang.Nullable` annotation was chosen to avoid discussion which `@Nullable` one is true one
    * `@NonNull` wasn't used as I didn't see much benefit of it. Anyhow, an empty constructor and/or setters allow to put a `null` value there

    Modifications:
    * Adds nullableAnnotation template to handle nullability annotation on vars
    * Adjust pojo templates to use the nullability template
    * Adapts tests

    Modifications:
    * Runs export_docs_generator.sh script to update samples

    * samples update

    * excludes Spring @Nullable from java-camel

    * ones with defaults shouldn't be annotated as @Nullable

    * updates samples

    * adds AllArgConstructor generation tests

    * adds container tests

which is #20345 in this case and this link to the PR with each commit listed in the commit tab.

I don't think Github will delete these commits but one can use web archive to archive these commit pages as backup.

Would this meet your requirement?

imaami · 2025-01-07T15:00:00Z

For merged PRs, the git message shows the PR number, e.g.
commit cba756ffa6b8a8ee15d456699fe36143a2f08f72 (HEAD -> master, origin/master, origin/HEAD)
Author: Andriy Slobodyanyk <[email protected]>
Date:   Mon Jan 6 16:08:01 2025 +0400

    Adds @Nullable annotation to Spring Boot generator (#20345)
which is #20345 in this case and this link to the PR with each commit listed in the commit tab.

I don't think Github will delete these commits but one can use web archive to archive these commit pages as backup.

Would this meet your requirement?

Archiving web pages would be entirely outside of the git workflow and not useful for any of the tools in git. If one can't use git log, git diff, git bisect, git revert etc. on the individual commits, then everything becomes manual and time consuming. Suppose you have a bug in a merge which is limited to some individual change in the original commit series, but the other commits in that series are OK. You can't use git revert anymore after a squash because there is no separate commit to revert in the master branch. You can only revert the whole thing, then manually piece together something that contains every change except that buggy one. Squashing will undo the usefulness of many of the best features git has to offer.

There's is one very inconvenient way to "import" the actual commits that all the squashed merges are based on. You can add a fetch entry to the git config like this:

[core]
	repositoryformatversion = 0
	filemode = true
	bare = false
	logallrefupdates = true
[remote "origin"]
	url = https://github.com/OpenAPITools/openapi-generator.git
	fetch = +refs/heads/*:refs/remotes/origin/*
	fetch = +refs/pull/*:refs/remotes/origin/pr/*
[branch "master"]
	remote = origin
	merge = refs/heads/master

The above snippet is .git/config with the added line fetch = +refs/pull/*:refs/remotes/origin/pr/*. After adding that you can fetch all the pre-merge remote branches with git fetch --all -tp.

It has its downsides though. Here's from before fetching the extra branches:

And here's after:

After fetching all of the extra refs the local repo is about 500 MiB larger, and there are now 12260 (!) more branches.

Even after all that it's still not as useful as git log simply showing the original commits. You have to go look for a PR number in the log, use that to get the PR branch name, then apply some shell trickery. It's doable, yes:

It's a cool trick now that I tried it myself, but it's still a hack that would be unnecessary if not for squashing. And of course none of this works if the local repo hasn't been configured to fetch all those PR branch refs from GitHub specifically. It depends entirely on GitHub being available and not deleting the refs from their servers.

Edit: one more detail: git revert, git bisect, etc. still won't work with the hack because the commits you can look at are not in the master branch. They're in dead-end side branches, all of them. In the end the only improvement is a slightly better reading experience.

imaami · 2025-01-07T15:12:23Z

I feel I need to clarify: I'm not demanding anything here! I want the project maintainers to make their own decisions about these things based on what they like best. I'm perfectly OK with any merge strategy, and it was actually fun to figure out that little hack. :)

imaami · 2025-01-07T19:23:26Z

I wrote a little script for listing "hidden" github commits associated with PR numbers.

#!/usr/bin/env bash

print_github_commits() {
	local b c n
	local -a commits

	commits=($(git log --pretty=%h "$@")) || return $?

	for c in "${commits[@]}"; do
		n=$(git log --pretty=%s -n1 "$c" \
		    | grep -o '(#[1-9][0-9]*)$')
		git log --oneline --color=always -n1 "$c"
		if [[ "$n" ]]; then
			b="origin/pr/${n:2:-1}/head"
			git log --oneline --color=always               \
			    $(git merge-base origin/master "$b").."$b" \
			| sed 's/^/  - /'
		fi
	done
}

print_github_commits "$@"

The syntax is mostly the same as for git log; you can do ./github-commits.sh -n5, ./github-commits.sh HEAD~6..HEAD etc.. Works only when the PR refs are added to the git config and fetched, and only works in the master branch.

eafer · 2025-01-07T23:13:25Z

My only objection to this pr is the sizeof style, but I guess it doesn't matter much. Otherwise all looks good.

imaami · 2025-01-08T08:19:54Z

My only objection to this pr is the sizeof style, but I guess it doesn't matter much. Otherwise all looks good.

I'll change the sizeof style and rebase one last time in an hour or two.

wing328 · 2025-01-08T08:29:37Z

I feel I need to clarify: I'm not demanding anything here! I want the project maintainers to make their own decisions about these things based on what they like best

thanks for the clarification. we continue with "squash and merge" as usual.

thanks for the feedback.

Don't explicitly cast void pointers as it's unnecessary in C, but leave printf arguments untouched to avoid setting off -Wformat.

wing328 · 2025-01-08T12:23:31Z

FYI. I've created a Slack channel to discuss anything related to the C client generator: https://openapi-generator.slack.com/archives/C087JQLBUHL/p1736329335975739. Feel free to join.

imaami · 2025-01-08T12:49:36Z

The failing check seems to be unrelated to this PR. ~~Looks like npm is trying to access https://registry.npmjs.org/typescript: instead of https://registry.npmjs.org/typescript for whatever reason.~~

Edit: Nope, the colon seems to be a bug in GitHub's link highlighting.

imaami · 2025-01-08T15:55:53Z

Just in case the failing TypeScript clients check is sporadic I'll re-force-push the head commit to trigger a build.

imaami · 2025-01-08T16:03:22Z

Nope, still fails. Also I realized that I misidentified what I thought was the problem. That stray colon in the URL seems to only be github's URL highlighter mistakenly appending a colon to the URL in the error message.

eafer · 2025-01-08T16:32:08Z

I'll change the sizeof style and rebase one last time in an hour or two.

I don't think you actually needed to rebase, the C generator doesn't get that many patches.

eafer · 2025-01-08T16:33:31Z

Sometimes I mess up the samples and I never know what went wrong, have you tried generating them again?

EDIT: never mind, you pr doesn't seem to have made any changes to typescript samples. No idea then.

wing328 · 2025-01-09T04:00:58Z

agreed https://github.com/OpenAPITools/openapi-generator/actions/runs/12674261348/job/35322526441?pr=20402 failure has nothing to do with this PR

wing328 added Issue: Bug Client: C labels Jan 6, 2025

imaami force-pushed the c-overhead-fixes branch from 7ca61f9 to 635ceb0 Compare January 6, 2025 20:57

eafer reviewed Jan 6, 2025

View reviewed changes

wing328 added this to the 7.11.0 milestone Jan 7, 2025

eafer approved these changes Jan 7, 2025

View reviewed changes

imaami added 2 commits January 8, 2025 13:27

[C][Client] Remove redundant casts

6f2fdab

Don't explicitly cast void pointers as it's unnecessary in C, but leave printf arguments untouched to avoid setting off -Wformat.

[C][Client] Cosmetic: remove unnecessary parens, align some lines

42516f6

imaami force-pushed the c-overhead-fixes branch from 635ceb0 to 99ef14e Compare January 8, 2025 12:24

[C][Client] Reduce number of unnecessary strlen() calls

7d87ff0

imaami force-pushed the c-overhead-fixes branch from 99ef14e to 7d87ff0 Compare January 8, 2025 15:58

wing328 merged commit 40d4703 into OpenAPITools:master Jan 9, 2025
18 of 19 checks passed

imaami deleted the c-overhead-fixes branch January 9, 2025 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C overhead fixes #20402

C overhead fixes #20402

imaami commented Jan 5, 2025

wing328 commented Jan 6, 2025

wing328 commented Jan 6, 2025

imaami commented Jan 6, 2025

eafer commented Jan 6, 2025

imaami commented Jan 6, 2025

imaami commented Jan 6, 2025

imaami commented Jan 6, 2025

eafer Jan 6, 2025

imaami Jan 7, 2025

eafer Jan 7, 2025

eafer commented Jan 6, 2025

wing328 commented Jan 7, 2025

imaami commented Jan 7, 2025 •

edited

Loading

imaami commented Jan 7, 2025

imaami commented Jan 7, 2025

eafer commented Jan 7, 2025

imaami commented Jan 8, 2025

wing328 commented Jan 8, 2025

wing328 commented Jan 8, 2025

imaami commented Jan 8, 2025 •

edited

Loading

imaami commented Jan 8, 2025

imaami commented Jan 8, 2025

eafer commented Jan 8, 2025

eafer commented Jan 8, 2025 •

edited

Loading

wing328 commented Jan 9, 2025

C overhead fixes #20402

C overhead fixes #20402

Conversation

imaami commented Jan 5, 2025

wing328 commented Jan 6, 2025

wing328 commented Jan 6, 2025

imaami commented Jan 6, 2025

eafer commented Jan 6, 2025

imaami commented Jan 6, 2025

imaami commented Jan 6, 2025

imaami commented Jan 6, 2025

eafer Jan 6, 2025

Choose a reason for hiding this comment

imaami Jan 7, 2025

Choose a reason for hiding this comment

eafer Jan 7, 2025

Choose a reason for hiding this comment

eafer commented Jan 6, 2025

wing328 commented Jan 7, 2025

imaami commented Jan 7, 2025 • edited Loading

imaami commented Jan 7, 2025

imaami commented Jan 7, 2025

eafer commented Jan 7, 2025

imaami commented Jan 8, 2025

wing328 commented Jan 8, 2025

wing328 commented Jan 8, 2025

imaami commented Jan 8, 2025 • edited Loading

imaami commented Jan 8, 2025

imaami commented Jan 8, 2025

eafer commented Jan 8, 2025

eafer commented Jan 8, 2025 • edited Loading

wing328 commented Jan 9, 2025

imaami commented Jan 7, 2025 •

edited

Loading

imaami commented Jan 8, 2025 •

edited

Loading

eafer commented Jan 8, 2025 •

edited

Loading