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

Build: devel: Replace indent with clang-format. #3824

Merged
merged 1 commit into from
Feb 17, 2025

Conversation

clumens
Copy link
Contributor

@clumens clumens commented Feb 10, 2025

The indent tool has fallen out of favor with a lot of developers, while clang-format seems to be growing in popularity. We're not enforcing the usage of any style tool because they never get everything 100% right, and I'm not really sure anyone ever actually uses this target - I know I never have.

However, this at least updates it to be something more modern should anyone want to use it. To generate this config, I dumped the default GNU coding style (clang-format -style=gnu -dump-config), removed anything that wasn't related to regular old C, and then went through every config option and checked to make sure it was close to our existing style. I'm sure something incorrect has snuck through.

The indent tool has fallen out of favor with a lot of developers, while
clang-format seems to be growing in popularity.  We're not enforcing the
usage of any style tool because they never get everything 100% right,
and I'm not really sure anyone ever actually uses this target - I know I
never have.

However, this at least updates it to be something more modern should
anyone want to use it.  To generate this config, I dumped the default
GNU coding style (clang-format -style=gnu -dump-config), removed
anything that wasn't related to regular old C, and then went through
every config option and checked to make sure it was close to our
existing style.  I'm sure something incorrect has snuck through.
@clumens
Copy link
Contributor Author

clumens commented Feb 14, 2025

Updated .clang-format to not reformat doxygen comment blocks.

@clumens
Copy link
Contributor Author

clumens commented Feb 17, 2025

@waltdisgrace Can you take a look at this? Thanks.

@clumens clumens merged commit 77a5fca into ClusterLabs:main Feb 17, 2025
1 check passed
@clumens clumens deleted the clang-format branch February 17, 2025 20:56
---
Language: Cpp
BasedOnStyle: GNU
AccessModifierOffset: -2
Copy link
Contributor

Choose a reason for hiding this comment

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

This one is C++-specific AFAICT

BasedOnStyle: GNU
AccessModifierOffset: -2
AlignAfterOpenBracket: Align
AlignArrayOfStructures: None
Copy link
Contributor

@nrwahl2 nrwahl2 Feb 18, 2025

Choose a reason for hiding this comment

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

If it matters, some of these options require particular minimum clang-format versions. For example, this one requires clang-format 13. The stakes are low, of course. Worst case scenario, this little-used developer make target breaks. Wouldn't hurt to have a note in INSTALL.md or a comment in this file or the Makefile about the minimum version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's true. In #3829, I brought up the fact that cppcheck changes what it can find and what needs to be suppressed over time too. I'm not sure if we've ever had any guidelines on what versions of these tools we're using and supporting. Given that it's all developer stuff, I lean towards not worrying about it too much. I expect that we're all using fairly modern stuff.

AlignAfterOpenBracket: Align
AlignArrayOfStructures: None
AlignConsecutiveAssignments:
Enabled: false
Copy link
Contributor

@nrwahl2 nrwahl2 Feb 18, 2025

Choose a reason for hiding this comment

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

Enabled: false covers all of these. Probably ditto for other option sets.

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.

3 participants