Various coding style fixes (no functional changes)#883
Conversation
|
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. |
|
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. |
|
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).
I guess that depends whether someone provides a PR. :) |
|
@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:
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. |
|
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. |
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.