Skip to content
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

Normalize and enforce CS in CI #674

Open
mvorisek opened this issue Jan 25, 2025 · 9 comments
Open

Normalize and enforce CS in CI #674

mvorisek opened this issue Jan 25, 2025 · 9 comments

Comments

@mvorisek
Copy link

I propose to use clang_format [1] CS formatter to enforce unified CS and minimize diff in a long term.

There are [2] multiple CS rulesets available out of the box matching well formed C CS by industry leaders.

The CS should be normalized in a few iterations (like WS first etc.) and then asserted by CI.

[1] https://clang.llvm.org/docs/ClangFormat.html
[2] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#basedonstyle

@NWilson
Copy link
Member

NWilson commented Jan 25, 2025

I'm afraid this is a "won't fix".

I understand why people feel strongly about code formatting (I do myself sometimes). For teams of 10 or more people working together, I would generally agree that using automated formatting is the way to go, to smooth things out for everyone.

However, we have a very small number of contributors to PCRE2 currently, and we are all happy enough with the current style. It is very unusual, and it took me a while to get used to, but I cannot see any advantage at the moment of reformatting.

It would not be possible to enforce the current style automatically either, since it is followed loosely in the current codebase.

Features and quality come first. The current formatting is not a barrier in any way to actual code improvements.

@mvorisek
Copy link
Author

Let's unify at least indentation and EOL.

@NWilson
Copy link
Member

NWilson commented Jan 25, 2025

EOL is LF throughout, and indentation is 2 spaces. Those are consistently followed already.

@PhilipHazel
Copy link
Collaborator

I am, of course, to blame for the current style. I adopted it when I first learned BCPL in 1968/9, before C existed. That's my excuse. However, any change now is up to @NWilson and other current contributors.

@zherczeg
Copy link
Collaborator

I would not mind to do some changes in the coding style, especially increasing the 80 column limit to 100 or 120.

@NWilson
Copy link
Member

NWilson commented Jan 27, 2025

That's fair. 120 columns is much more standard nowadays, and wrapped lines are far less readable than unwrapped lines.

Let's use 120 columns from now on in new code, and worry about re-wrapping existing code at some future point.

I had one extra thought: it's possible that younger developers see the code, and think "ugh, legacy C code" and are put off, or feel unwelcome to contribute. That's unfortunate, but perhaps understandable. It's completely unknowable, and it seems ridiculous, but we might get more contributors if we just moved the indentation of the braces to something more common.

@zherczeg
Copy link
Collaborator

I like the idea of changing the braces, that is probably the most unusal part of the coding style. I am not sure any tool can do that, so we would probably need some scripts to do it though.

@mvorisek
Copy link
Author

ClangFormat is very configurable, as long as the current CS unified enough, ClangFormat can be configured to match the current CS.

In the ClangFormat docs there is:

clang-format -style=llvm -dump-config > .clang-format

This should generate .clang-format file will all the options of the base CS (llvm in the example). The individual format options can be then tweaked to match the current CS.

Surely, there will be some minimal unavoidable changes. But once this is done, the CS can be fully asserted by CI and modernized like improved placement of the braces.

@mvorisek mvorisek reopened this Jan 30, 2025
@NWilson
Copy link
Member

NWilson commented Jan 31, 2025

We won't be using clang-format, because it's too strict: it normalises absolutely everything, and preserves nothing from the input formatting. The existing code is artistically formatted, with things lined up by eye to look nice. If we do change the formatting, we'd need to use a more lenient tool.

We also won't be enforcing style via CI. There are ~four people who contribute to the code, and I see no reason to impose any changes on their workflow.

I will leave the ticket open for us to consider updating the braces style. More modern visual appearance may make it easier to attract contributors.

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

No branches or pull requests

4 participants