Skip to content

Various coding style fixes (no functional changes)#883

Open
thg2k wants to merge 1 commit into
libgd:masterfrom
thg2k:pr/cs1
Open

Various coding style fixes (no functional changes)#883
thg2k wants to merge 1 commit into
libgd:masterfrom
thg2k:pr/cs1

Conversation

@thg2k

@thg2k thg2k commented Jun 9, 2024

Copy link
Copy Markdown

Testing the water for these kind of CS fixes, if they are welcome I would submit a lot more as i'm doing a walk through of the source code.

@cmb69

cmb69 commented Dec 21, 2024

Copy link
Copy Markdown
Member

I've just skimmed the patch, and it looks good to me, but I think we should establish some coding style rules first – otherwise we might go back and forth forever.

@thg2k thg2k mentioned this pull request Dec 27, 2024
@thg2k

thg2k commented Dec 27, 2024

Copy link
Copy Markdown
Author

I've just skimmed the patch, and it looks good to me, but I think we should establish some coding style rules first – otherwise we might go back and forth forever.

I agree it would be beneficial to have coding style rules, but I don't know if, when, and how this can happen. In theory, this patch addresses only obvious inconsistencies. If something doesn't look right, I'm happy to take it out so that the non-controversial stuff can be merged. Just mark the lines that you don't like and i'll revert them.

@vapier

vapier commented Jan 7, 2025

Copy link
Copy Markdown
Member

an agreed upon style is needed, but even more so, an automatic formatter that is checked in CI. clang-format is the obvious candidate. I don't think we want to do mass-formats until that's in place to avoid more churn.

@thg2k

thg2k commented Jan 7, 2025

Copy link
Copy Markdown
Author

an agreed upon style is needed, but even more so, an automatic formatter that is checked in CI. clang-format is the obvious candidate. I don't think we want to do mass-formats until that's in place to avoid more churn.

@vapier Do you have an estimated timeline for when that could happen? Did you see the actual changes in this PR? Do you believe these changes would go in the wrong direction according to your future established coding styles rules?

I recently saw a PR where you commented on coding style, I believe my PR brings the codebase in the same direction, but if you can point out details I'm happy to amend it.

Other options would be to keep this PR stalled until coding style / CI is in place or simply discard and close it.

Let me know, thank you.

@cmb69

cmb69 commented Jan 7, 2025

Copy link
Copy Markdown
Member

New code, or maybe code that is modified, should use some "generally" agreed conventions (e.g. now empty statement after a compound statement, i.e. no semicolon after closing brace). That's different from mass reformatting, which can cause merge conflicts (some commits may later be cherry-picked to a different branch, and some branches may need to be kept in sync). And we also should not make it unreasonably hard to maintain forks (e.g. PHP's bundled libgd).

Do you have an estimated timeline for when that could happen?

I guess that depends whether someone provides a PR. :)

@thg2k

thg2k commented Dec 22, 2025

Copy link
Copy Markdown
Author

@cmb69 I fully agree with your point, but let me explain the origin of this PR. Apologies for the long text.

Some time ago I wanted to assess the actual behavioral differences between official libgd and PHP's bundled version. So I tried a naive diff of the sources, but the result was unreadable due to noise of purely non-functional changes. Over many years, functional changes have been cherry picked or directly made on PHP's side, while formatting-only changes were never ported. The result is that PHP's side of GD is a frankenstein version between 2.0, 2.2, and master. So I started to manually merge onto's PHP side all non-functional changes, to reduce the noise and obtain a diff with purely functional changes. This would have resulted in a PR at PHP's side. But I ran into a problem: I realized that on some parts, the coding style on the PHP's side was actually better than the official libgd's one (see this PR for actual examples). And why is this a problem? Because to complete my quest, I would have to actually worsen the coding style on PHP's side, which is a bit ridiculous. That's why led to this PR.

So my war plan would be:

  1. Manually merge only non-functional differences between PHP master and GD master, picking the side with the "best" coding style for each difference.
  2. Submit the resulting PR on GD master (note: this is only a partial one).
  3. Submit the resulting PR on PHP master after merging "fixes" on GD master.

Does this make sense? The ultimate goal is to give PHP people a way to analyze the functional differences before moving forward for a possible unbundling of GD or full-sync.

If possible, i'd also like to hear @devnexen take on this, as he is also involved.

@devnexen

Copy link
Copy Markdown
Contributor

Ideally, we always thrive for lesser deltas, but it is very fair to expect using tools such as clang-format, needs to choose a agreed format very likely a custom one ; "why 3 spaces instead of 4 ?" and so on. Thankfully, most of the changes from here are fairly easy to backport in php side.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants