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

Modernize clang-format file #342

Draft
wants to merge 20 commits into
base: master
Choose a base branch
from
Draft

Modernize clang-format file #342

wants to merge 20 commits into from

Conversation

HomesGH
Copy link
Contributor

@HomesGH HomesGH commented Sep 23, 2024

Description

As discussed in issue #276 , we want to add a clang-format file to enforce an unified style in ls1.
For this, we have to:

  • Agree on the content of .clang-format file
  • Agree on clang-format version (and add it to CI file)
  • Add CI to check format (DISARMED FOR NOW)
  • Add target to CMake to just run command make clang-format for formatting (this is not working for now, help wanted!)
  • Add target to Makefile to just run command make clang-format for formatting

If we want to be very fancy, we could do something like in MegaMol.

Resolved Issues

I would recommend that this PR does not apply the formatting to the whole code base in ls1. This should be done in a separate PR for the sake of clarity.

@HomesGH HomesGH changed the title Addclangformat Modernize clang-format file Sep 23, 2024
@HomesGH HomesGH marked this pull request as draft September 23, 2024 07:45
@amartyads
Copy link
Contributor

amartyads commented Sep 23, 2024

I like that the clangformat file only has the essentials for now, I think it makes it easier for discussion purposes. In the final version, however, we should change it back into a full file.

I made the BreakBeforeBraces style Attach, now we will have } else { again instead of else on a newline.

One (potentially) controversial change I've made is PointerAlignment: Left. We prefer Right (I do too), but the PDF style guide says Left, and I think that if we're following the PDF on spaces vs tabs, we should follow it here as well.

  • Change .clang-format file back to a full config file, from a cropped one

The CI seems to be using clangformat 14. I would vastly prefer 18, because that's what MaMiCo uses, but we can discuss that further.

We could also use git hooks to format all new files according to the clangformat, however AFAIK git hooks are not server side.

Copy link

⚠️ The CI detected formatting issues in this pull request. Please run clang-format locally to resolve them.
Details are shown in the job log.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enforce formatting through jenkins
2 participants