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" clang format #42

Merged
merged 1 commit into from
Jan 22, 2024
Merged

applied "google" clang format #42

merged 1 commit into from
Jan 22, 2024

Conversation

mkalkbrenner
Copy link
Collaborator

@mkalkbrenner mkalkbrenner commented Jan 21, 2024

Reformatted the code using the "google" coding style.
Added a clang format file that ensures that our IDEs will format code consistently like this in the future.

Agreeing on a coding style is important to keep PRs readable. A PR will then just show the real differences and no reformatting changes caused by different settings in our IDEs.

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

@mkalkbrenner
Copy link
Collaborator Author

The code hasn't been changed, just reformatted. The builds could be checked here:
https://github.com/zesinger/libserum/actions/runs/7601025625

@jsm174
Copy link
Contributor

jsm174 commented Jan 22, 2024

I'd like to see this too, as they are so related to libzedmd and zedmd. Consistency is always good.

I do have two other thoughts that maybe someday would be nice to consider?

  • run the code through a static analysis tool. @toxieinc runs all the vpinball/pinmame projects through one and it is absolutely amazing at what it finds. example 1 example 2

  • convert to a true c++ class. Everything is global right now, so we can really only have one per executable. I know realistically no one would ever need multiple, but in my case, we moved alot of the dmd logic out of vpx standalone and into a new library called libdmdutil. Many pinmame based vpx tables have a dmd for pinmame and then a flexdmd for table options. Some actually fire up two vpinmame controllers (still not sure why on this one). I had to wrap serum in a class (see here), just to prevent a second load and keep track of the buffer for changes and rotations.

@mkalkbrenner
Copy link
Collaborator Author

I'd like to see this too, as they are so related to libzedmd and zedmd. Consistency is always good.

@zesinger it would be great if you merge our request as it is.

I do have two other thoughts that maybe someday would be nice to consider?

* run the code through a [static analysis tool](https://pvs-studio.com/en/order/open-source-license/). @toxieinc runs all the vpinball/pinmame projects through one and it is absolutely amazing at what it finds. [example 1](https://github.com/vpinball/libdmdutil/commit/ddd305af193c1d0aa19afc8763f6ae442bec72e8) [example 2](https://github.com/vpinball/vpinball/commit/2ff973507b6fdca18ff30e4f56b003bd2d7203ba)

I agree and I already use static code analysis for other projects. But we should open a dedicated issue for that. And applying a coding style should be committed first to better understand these findings.

* convert to a true c++ class. Everything is global right now, so we can really only have one per executable. I know realistically no one would ever need multiple, but in my case, we moved alot of the dmd logic out of vpx standalone and into a new library called libdmdutil.  Many pinmame based vpx tables have a dmd for pinmame and then a flexdmd for table options. Some actually fire up two vpinmame controllers (still not sure why on this one). I had to wrap serum in a class [(see here)](https://github.com/vpinball/libdmdutil/blob/b300cd64060b3dd995341087b22a83f414f8b513/src/Serum.cpp#L1-L74), just to prevent a second load and keep track of the buffer for changes and rotations.

I also support this request. But it should be a dedicated issue, too.

@mkalkbrenner
Copy link
Collaborator Author

as discussed with @zesinger I go ahead and just merge the PR. It doesn't do anything but formatting the code according to google's coding standard.

@mkalkbrenner mkalkbrenner merged commit 400787b into zesinger:main Jan 22, 2024
11 checks passed
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.

2 participants