Skip to content

Clang format #1273

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 2 commits into
base: dev-v1.0
Choose a base branch
from
Draft

Clang format #1273

wants to merge 2 commits into from

Conversation

ZehMatt
Copy link
Contributor

@ZehMatt ZehMatt commented Aug 17, 2023

I've tried to minimize the diff but also didn't want to exactly match it to get somewhere in the middle of readability and consistency. I also enabled the option to auto comment the end of the namespace, this is a bit inconsistent at the moment and we should either turn that option off or remove the ones that don't fit, I personally think its best to let clang-format do the commenting so it won't be missed and it will be consistent across the board.

Probably need a few more files I should look into, clang-format can get quite notorious about tables.

@ZehMatt ZehMatt marked this pull request as draft August 17, 2023 13:13
@SweetVishnya
Copy link
Contributor

We could also add clang-format checks to CI

@ZehMatt
Copy link
Contributor Author

ZehMatt commented Aug 17, 2023

We could also add clang-format checks to CI

I could do that but its best to have all of the files formatted first

@JonathanSalwan
Copy link
Owner

Thanks for this MR,

I could do that but its best to have all of the files formatted first

Before updating all files, we should first agree on a format :). I will play with the example you provided and get back to you as soon as I can.

@ZehMatt
Copy link
Contributor Author

ZehMatt commented Aug 17, 2023

Before updating all files, we should first agree on a format :).

Definitely, that is also what I meant.

I will play with the example you provided and get back to you as soon as I can.

No rush here, thanks for looking into it.

@cnheitman
Copy link
Collaborator

This is a nice addition :)

@ZehMatt Could you modify the PR so it merges into dev-v1.0 instead of master?

@ZehMatt ZehMatt changed the base branch from master to dev-v1.0 August 25, 2023 14:24
@ZehMatt
Copy link
Contributor Author

ZehMatt commented Aug 25, 2023

This is a nice addition :)

@ZehMatt Could you modify the PR so it merges into dev-v1.0 instead of master?

Not sure why GitHub decided to pick master, I branched off dev-1.0 🤷‍♂️ , changed it.

@cnheitman
Copy link
Collaborator

Thanks, @ZehMatt !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ZehMatt @SweetVishnya @JonathanSalwan @cnheitman