search

vpinball / libdmdutil

A cross platform library for performing DMD tasks.
Other
3 stars 8 forks source link

applied google's clang format #3

Closed mkalkbrenner closed 9 months ago

mkalkbrenner commented 9 months ago

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 commented 9 months ago

https://google.github.io/styleguide/cppguide.html

jsm174 commented 9 months ago

@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 commented 9 months ago

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.

toxieainc commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

mkalkbrenner commented 9 months ago

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

toxieainc commented 9 months ago

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 commented 9 months ago

I can do that.

So just to confirm:

BreakBeforeBraces: Allman
ColumnLimit: 120
jsm174 commented 9 months ago

@toxieainc @mkalkbrenner - can you take a look at https://github.com/vpinball/libdmdutil/commit/640912bac1c509d5149788142e50064a30c1cc03

I used the source from https://github.com/vpinball/libdmdutil/commit/b300cd64060b3dd995341087b22a83f414f8b513 so immediately before the first apply. I think thats what you were asking to do.

toxieainc commented 9 months ago

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

jsm174 commented 9 months ago

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

jsm174 commented 9 months ago

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

toxieainc commented 9 months ago

Neat, thanks!!