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

Switch to clang-format #718

Merged
merged 4 commits into from
Jan 20, 2025
Merged

Switch to clang-format #718

merged 4 commits into from
Jan 20, 2025

Conversation

flichtenheld
Copy link
Member

The uncrustify tool is relatively unstable, the configuration is difficult to understand.

Switch to clang-format instead, which has better support in terms of maintenance and integrations.

The clang-format configuration I have developed to be as close to old "OpenVPN" style as enforced by uncrustify as possible. While switching openvpn2 itself to it is a bigger project, I think this project should be easier.

I have skimmed over the reformat commit and didn't see any real cases where it made the code worse, but feel free to point out any specific instances.

@selvanair
Copy link
Collaborator

selvanair commented Jan 16, 2025

This does do a few arguably weird things: E.g.,
(i) split function arguments to one per line in some places (e.g., line ~151, ~260 in access.c), but consolidate them to one line in other (line ~230 in access.c)
(ii) line break right after opening bracket in some function calls (e.g., line ~385 in access.c)
But I do not have a strong opinion. Works for me.

I compared the resulting object files --- no differences found other than expected from the use of __LINE__ and assert() in a few places.

BTW:
(i) ubuntu 22.04 has clang-format version14 which does not support AlignConsecutiveMacros
(ii) This PR created a new branch named clang-format here -- is that by mistake?

@lstipakov
Copy link
Member

VS supports clang format natively, so it is definitely an improvement compared to uncrustify.

@flichtenheld
Copy link
Member Author

This does do a few arguably weird things: E.g., (i) split function arguments to one per line in some places (e.g., line ~151, ~260 in access.c), but consolidate them to one line in other (line ~230 in access.c) (ii) line break right after opening bracket in some function calls (e.g., line ~385 in access.c) But I do not have a strong opinion. Works for me.

Are you sure these line numbers are from access.c? I do not see the examples you point to there?

I compared the resulting object files --- no differences found other than expected from the use of __LINE__ and assert() in a few places.

BTW: (i) ubuntu 22.04 has clang-format version14 which does not support AlignConsecutiveMacros

Yeah, in other projects we tried to work with the clang-format from distros, but that is very limiting, in how old that always ends up being. And while clang-format is generally stable, it still sometimes changes behavior between versions, e.g. due to bug fixes. This is why I did go the route of adding the pre-commit support, which allows you to specify a specific clang-format version that it will then always use. I think it installs it via a pypi package? Would need to check. This way works for me, for people with IDEs they will need to check how to get the latest clang-format. But definitely no to using the clang-format from a specific Ubuntu version.

(ii) This PR created a new branch named clang-format here -- is that by mistake?

Hmm, yeah, I could have used a fork. I had the permissions so I did it without much thought.

Copy link
Collaborator

@selvanair selvanair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My line number indications were poor. Marked some inline this time.
That said, LGTM given uncrustify is more of a pain.

cmd, params, GetLastError());
cmd,
params,
GetLastError());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a case where it splits args each into a new line.

access.c Show resolved Hide resolved
err = NetLocalGroupGetMembers(NULL, group_name, 0, (LPBYTE *) &members,
MAX_PREFERRED_LENGTH, &nread, &nmax, &resume);
err = NetLocalGroupGetMembers(
NULL, group_name, 0, (LPBYTE *)&members, MAX_PREFERRED_LENGTH, &nread, &nmax, &resume);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an example of line break after opening "("

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, one can play around with the different penalty values like PenaltyBreakBeforeFirstCallParameter to influence this. And obviously the ColumnLimit plays a huge role here.

This tries to capture the "OpenVPN" format as
good as possible.

Signed-off-by: Frank Lichtenheld <[email protected]>
Do not use the old uncrustify hook anymore.

Signed-off-by: Frank Lichtenheld <[email protected]>
@flichtenheld flichtenheld merged commit 4318e31 into master Jan 20, 2025
20 checks passed
@flichtenheld flichtenheld deleted the clang-format branch January 20, 2025 12:40
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.

3 participants