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.

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

STYLE: Consistent formatting and indentation of C++ sources #7603

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

STYLE: Consistent formatting and indentation of C++ sources #7603

wants to merge 3 commits into from

Conversation

jcfr
Copy link
Member

@jcfr jcfr commented Feb 23, 2024

This pull request ensures consistent indentation and formatting of for C++ source files.

It introduces clang-format configuration files as well as pre-commit configuration ensuring that IDE can appropriately format the code and continuous integration workflow can report when improperly formatted code is contributed.

Once integrated, this commit will be added to the .git-blame-ignore-revs1 file.

Open Questions

  • Should we set AlignConsecutiveAssignments2 to true ?
  • Should we customized the penalties3 ?
  • Should we add additional clang-format exceptions ?

Penalties

Default value for penalties are defined as such4:

  LLVMStyle.PenaltyBreakAssignment = prec::Assignment;
  LLVMStyle.PenaltyBreakComment = 300;
  LLVMStyle.PenaltyBreakFirstLessLess = 120;
  LLVMStyle.PenaltyBreakString = 1000;
  LLVMStyle.PenaltyExcessCharacter = 1000000;
  LLVMStyle.PenaltyReturnTypeOnItsOwnLine = 60;
  LLVMStyle.PenaltyBreakBeforeFirstCallParameter = 19;
  LLVMStyle.PenaltyBreakOpenParenthesis = 0;
  LLVMStyle.PenaltyBreakTemplateDeclaration = prec::Relational;
  LLVMStyle.PenaltyIndentedWhitespace = 0;

Footnotes

  1. https://github.com/Slicer/Slicer/blob/main/.git-blame-ignore-revs ↩

  2. https://clang.llvm.org/docs/ClangFormatStyleOptions.html#alignconsecutiveassignments ↩

  3. https://clang.llvm.org/docs/ClangFormatStyleOptions.html#penaltybreakassignment ↩

  4. https://github.com/llvm/llvm-project/blob/llvmorg-17.0.6/clang/lib/Format/Format.cpp#L1559-L1568 ↩

@jcfr jcfr marked this pull request as draft February 23, 2024 22:37
@jcfr jcfr force-pushed the style-consistently-indent-cpp-files branch from 53355b9 to 6879424 Compare February 23, 2024 22:39
jcfr
jcfr commented Feb 23, 2024
View reviewed changes
Base/QTGUI/qSlicerModuleSelectorToolBar.cxx
QObject::connect(q,
SIGNAL(toolButtonStyleChanged(Qt::ToolButtonStyle)),
this->ModuleFinderButton,
SLOT(setToolButtonStyle(Qt::ToolButtonStyle)));
Copy link
Member Author

Choose a reason for hiding this comment

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

Are we okay with this type of change ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

@jcfr
ENH: Add clang-format configuration & associated exceptions
459e92d 
Configuration based of Clang v17.0.6

Exceptions identified by visually inspecting the updates after
running `pre-commit run clang-format -a`

Running pre-commit required to update the pre-commit config applying the
following patch:

```diff
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 2965afd3d3..8016726ea7 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -21,3 +21,8 @@ repos:
   hooks:
   - id: pyupgrade
     args: [--py39-plus]
+
+- repo: https://github.com/pre-commit/mirrors-clang-format
+  rev: "v17.0.6"
+  hooks:
+  - id: clang-format
```
@jcfr
STYLE: Consistent formatting of C++ sources using clang-format
0a19cb2 
Sources updated based on the clang-format configuration introduced in the
previous commit.
@jcfr
ENH: Update pre-commit config to include "clang-format"
2a72244 
By adding `clang-format` to the pre-commit configuration, it enables
the following:
* GitHub Action report: Suggested changes are reported in the error log
  associated with the pre-commit GitHub Action workflow.
* Consolidated local pre-commit checks and updates by running:

  ```
  pip install pre-commit
  pre-commit run --all-files
  ```
@jcfr jcfr force-pushed the style-consistently-indent-cpp-files branch from 6879424 to 2a72244 Compare February 24, 2024 00:07
@jcfr jcfr marked this pull request as ready for review February 24, 2024 00:08
pieper
pieper reviewed Feb 26, 2024
View reviewed changes
Copy link
Member

@pieper pieper left a comment

Choose a reason for hiding this comment

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

I spot checked and didn't see anything I can't live with.

@cpinter
Copy link
Member

cpinter commented Feb 27, 2024

Thanks @jcfr it looks good!

I would not enable AlignConsecutiveAssignments because when we rename variables the alignment falls apart, and I prefer not to have anything badly aligned as opposed to not aligned at all. Cleaner in terms of number of unnecessary characters too.

I don't have specific suggestion about the penalties, anything reasonable will be fine with me.

@jcfr jcfr marked this pull request as draft February 27, 2024 22:40
@jcfr
Copy link
Member Author

jcfr commented Feb 27, 2024

Following today's Slicer weekly meeting1, we decided to delayed the integration.

We will first work to incrementally integrate formatting changes using uncrustify, this will help reduce the diff and experiment with different options to better assess the impact of line break changes specific to clang-format.

Footnotes

  1. https://discourse.slicer.org/t/2024-02-27-weekly-meeting/34553 ↩

@jcfr
Copy link
Member Author

jcfr commented Feb 27, 2024

Thanks @pieper, @cpinter, @lassoan @RafaelPalomar, @jamesobutler and @sjh26 for the review and feedback 🙏

@jcfr jcfr added the Status: Draft This pull-request is not yet ready for integration label Mar 6, 2024
@jcfr jcfr mentioned this pull request Mar 14, 2024
Open
@hjmjohnson
Copy link
Member

Consistent automatic formatting is a great improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pieper pieper pieper left review comments

@hjmjohnson hjmjohnson hjmjohnson left review comments

@lassoan lassoan Awaiting requested review from lassoan

@jamesobutler jamesobutler Awaiting requested review from jamesobutler

@sjh26 sjh26 Awaiting requested review from sjh26

@cpinter cpinter Awaiting requested review from cpinter

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Status: Draft This pull-request is not yet ready for integration
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jcfr @cpinter @hjmjohnson @pieper