Skip to content

Conversation

@MinyazevR
Copy link
Contributor

@MinyazevR MinyazevR commented Nov 14, 2025

Full description: #265

Copy link
Contributor

@mrbean-bremen mrbean-bremen left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good! I would like a couple of minor changes, and would like @usiems to have another look, before merging this.

.clang-format Outdated
SplitEmptyNamespace: true
PPIndentWidth: 2
FixNamespaceComments: false
# ??????
Copy link
Contributor

Choose a reason for hiding this comment

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

?

.clang-format Outdated
ReflowComments: false
ConstructorInitializerIndentWidth: 2
ContinuationIndentWidth: 2
AllowShortFunctionsOnASingleLine: None
Copy link
Contributor

Choose a reason for hiding this comment

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

I would propose to use
AllowShortFunctionsOnASingleLine: Inline
here.

This way short inline functions in the header are put into one line.

.clang-format Outdated
AfterFunction: true
AfterNamespace: false
AfterEnum: false
AfterCaseLabel: false
Copy link
Contributor

Choose a reason for hiding this comment

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

'AfterCaseLabel: true' better matches the current style.

I rather dislike that the break behind a closing brace is put onto the same line, but found no option to influence this. The developers of clang-format seem to believe that this is as it should be.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is the option BreakBeforeBraces: Allman which would put all braces on a separate line, including the break. I would be fine with this, but it would be a larger change for the existing code base.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah, I don't like that, in my opinion this takes up too much space.

.clang-format Outdated
AfterNamespace: false
AfterEnum: false
AfterCaseLabel: false
AfterControlStatement: Never
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like this to be
AfterControlStatement: MultiLine
that is, put the brace on the next line if the if condition has more than one line.

.clang-format Outdated
PadOperators: false
AlignEscapedNewlines: DontAlign
AlignOperands: Align
AlignTrailingComments: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Setting
AlignTrailingComments: true
neatly arranges the comments in the initialization lists of Python objects.

Speaking of initializer lists of Python objects: I noticed that clang-format collapses lines without trailing comments in these lists, which makes these less tidy. We should add trailing comments for these lines to keep the Python style (search for _HEAD_INIT in the code).

.clang-format Outdated
AllowShortBlocksOnASingleLine: Empty
AllowShortCaseLabelsOnASingleLine: false
AllowShortEnumsOnASingleLine: true
# AllowShortIfStatementsOnASingleLine: Nevers
Copy link
Contributor

Choose a reason for hiding this comment

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

The value for this option is Never!

Copy link
Contributor

@mrbean-bremen mrbean-bremen left a comment

Choose a reason for hiding this comment

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

Looks good from my perspective, thanks!
I leave it to @usiems to decide about the BreakBeforeBraces: Allman option.

Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to consider adding the clang-format hook to a pre-commit-config yaml as 3D Slicer and other related projects have adopted (see https://github.com/Slicer/Slicer/blob/1b1bbb22b4f858ff3d47d196de4ac491a0b9ea5c/.pre-commit-config.yaml#L27-L39). Pre-commit-config yml usage has been adopted by many projects and something that is generally called in a GitHub actions workflow file (e.g. lint.yml) using the pre-commit github action (https://github.com/pre-commit/action).

^This way new styling is applied consistently and enforced for every PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, good point! I'm using pre-commit in all Python projects, just didn't think about it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jamesobutler - pre-commit is added now, thanks for the suggestion!

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! Now just need a lint github workflow to run pre-commit where the workflow will fail if pre-commit detects issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had also enabled pre-commit.ci (as I do usually with pre-commit), so indeed the workflow will fail in this case.

@usiems usiems merged commit 89fe8d2 into MeVisLab:main Nov 19, 2025
17 checks passed
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