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

applied google's clang format #3

Merged
merged 2 commits into from
Jan 22, 2024
Merged

applied google's clang format #3

merged 2 commits into from
Jan 22, 2024

Conversation

mkalkbrenner
Copy link
Collaborator

@mkalkbrenner mkalkbrenner commented Jan 22, 2024

Before I start to migrate PPUC's threading model and PIN2DMD support to libdmdutil, it would be important to ensure a common coding style.
This PR applies google's coding standard, the one we agreed on for libserum, libzedmd, libppuc, ...
And I changed all line endings to LF to be consistent within the project.

@mkalkbrenner
Copy link
Collaborator Author

@jsm174 jsm174 requested a review from toxieainc January 22, 2024 18:08
@jsm174
Copy link
Collaborator

jsm174 commented Jan 22, 2024

@toxieainc, are you using a clang format when doing some of the static analysis cleanup? since this project is under the vpinball org, I'd go for whatever is consistent.

@toxieainc
Copy link
Member

Static analysis is kinda orthogonal to coding guidelines/formatting.

For vpinball and pinmame we have no real guidelines/common formatting yet, only 'adapt to guidelines/formatting of surrounding code to be locally consistent'. And i'm against unifying it as an afterthought, as then the blame functionality is always (at least) one more step complicated.
But as libdmdutil is still so new it will not matter much, so if there is already a 'standard' between 'neighboring' libs please go ahead with that.

Copy link
Member

@toxieainc toxieainc left a comment

Choose a reason for hiding this comment

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

I'm no big fan of such things, but also don't care much.

@jsm174 jsm174 merged commit 3bc4ee0 into vpinball:master Jan 22, 2024
11 checks passed
@toxieainc
Copy link
Member

One additional comment @jsm174 @mkalkbrenner now that i see the changes in practice.
Is it really necessary to limit line lengths that harshly in 2024, 4k screens and all? ;)
So that part i would personally change again.

@mkalkbrenner
Copy link
Collaborator Author

I just applied a a coding style that is available out of the box. But for sure we can adjust the clang format file according to our needs.

@mkalkbrenner
Copy link
Collaborator Author

mkalkbrenner commented Jan 24, 2024

But i must admit, that shorter lines have advantages in diff views, especially in three way diffs, even on my ultra wide screen. And I use diff views very often for reviews and especially if I start contributing to something new.
But that is just my opinion.

@jsm174
Copy link
Collaborator

jsm174 commented Jan 24, 2024

I have a really hard time with super super super short lines like this one does out of the box as well.

@mkalkbrenner
Copy link
Collaborator Author

@jsm174 @toxieainc
So what is the conclusion? Raising the limit but keeping the other rules?

@toxieainc
Copy link
Member

I personally would vote for 120.

Plus i personally prefer BreakBeforeBraces Allman (=always do a linebreak before braces), but i know that this setting in general is always debated. At least from my own experience (ranging from tiny open source projects to double-digit-million-line professional projects) this was and is the most common though.

If any of these is applied, i guess best would be to revert to pre-PR state and then apply, otherwise some existing new linebreaks may be kept?

@jsm174
Copy link
Collaborator

jsm174 commented Jan 24, 2024

I can do that.

So just to confirm:

BreakBeforeBraces: Allman
ColumnLimit: 120

@jsm174
Copy link
Collaborator

jsm174 commented Jan 25, 2024

@toxieainc @mkalkbrenner - can you take a look at 640912b

I used the source from b300cd6 so immediately before the first apply. I think thats what you were asking to do.

@toxieainc
Copy link
Member

Looks better, yes, thanks!
IMHO there is only one piece that looks worse now: AlphaNumeric::SegSizes definition. But thats negligible.

@jsm174
Copy link
Collaborator

jsm174 commented Jan 25, 2024

there might be a way to suppress clang format with a tag or something. I will check

@jsm174
Copy link
Collaborator

jsm174 commented Jan 25, 2024

@toxieainc - pushed again with // clang-format off and // clang-format on

@toxieainc
Copy link
Member

Neat, thanks!!

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