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

Makefile: add clang-format #176

Merged
merged 2 commits into from
Oct 3, 2024
Merged

Makefile: add clang-format #176

merged 2 commits into from
Oct 3, 2024

Conversation

dkropachev
Copy link
Collaborator

It adds clang-format to the list of linters for c/cpp code.
Also it applies format changes suggested by clang-format

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have enabled appropriate tests in .github/workflows/build.yml in gtest_filter.
  • I have enabled appropriate tests in .github/workflows/cassandra.yml in gtest_filter.

@Lorak-mmk
Copy link
Collaborator

I wonder if this this something that should be done in this project. C/Cpp files are only test-related, copied verbatim from original cpp-driver and modified a bit. Maybe it's of more value to reduce amount of modifications to them, so that it is easier to diff them eith their original versions?

@Lorak-mmk
Copy link
Collaborator

Another issue: biggest cause for automated code formatting is consistency of formatting. This PR uses default style of clang-format, without any config. This is consistent neither with scylla / seastar style, nor with original cpp-driver style.
Imo if we were to add automated formatting it should mimic one of those style as close as possible. Personally I'd be for the style of original cpp-driver, becuse it will make it easier to read both codebases together (which is something I know I had to do many times when developing this project).

@muzarski
Copy link
Collaborator

muzarski commented Sep 20, 2024

There is a clang-format config file in cpp-driver's repo: https://github.com/datastax/cpp-driver/blob/master/.clang-format. We should definitely use it,

@dkropachev
Copy link
Collaborator Author

There is a clang-format config file in cpp-driver's repo: https://github.com/datastax/cpp-driver/blob/master/.clang-format. We should definitely use it,

Thanks, I have copied it.
@Lorak-mmk , does it address your concerns ?

@Lorak-mmk
Copy link
Collaborator

How did you generate the last commit? It looks like it doesn't respect the config. Example: look at src/aligned_storage.hpp. Comment was reflown in your PR, despite not being longer than 100 characters (which is the limit in config

I checked out the PR, dropped the last commit and executed make fix-clang-format - this seems to result in correct changes (I don't see wrongly reflown comments).

Another issue: I think that third_party folder should be skipped. Those are basically dependencies, not something that we should edit.

@dkropachev
Copy link
Collaborator Author

How did you generate the last commit? It looks like it doesn't respect the config. Example: look at src/aligned_storage.hpp. Comment was reflown in your PR, despite not being longer than 100 characters (which is the limit in config

I checked out the PR, dropped the last commit and executed make fix-clang-format - this seems to result in correct changes (I don't see wrongly reflown comments).

Another issue: I think that third_party folder should be skipped. Those are basically dependencies, not something that we should edit.

Thanks, both done, third_party excluded, last commit removed and formatting reapplied.

Copy link
Collaborator

@muzarski muzarski left a comment

Choose a reason for hiding this comment

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

Please squash first and second commits, otherwise LGTM

@dkropachev
Copy link
Collaborator Author

Please squash first and second commits, otherwise LGTM

@Lorak-mmk , are you ok with it ?

@muzarski muzarski merged commit acc07fc into master Oct 3, 2024
7 checks passed
@muzarski muzarski mentioned this pull request Oct 3, 2024
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.

4 participants