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

Update to clang format 19 support #5015

Merged
merged 4 commits into from
Dec 6, 2024
Merged

Update to clang format 19 support #5015

merged 4 commits into from
Dec 6, 2024

Conversation

hjmjohnson
Copy link
Member

Resolves: #4565

Update to newer version of clang-format.

The previous clang-format version 8 does not compile on arm64 mac. There is limited support for clang-format binaries before version 10.

This is a gateway toward greatly simplifying the support for pre-commit checking using the python pre-commit infrastructure. See: #5013

PR Checklist

@hjmjohnson hjmjohnson added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Style Style changes: no logic impact (indentation, comments, naming) labels Dec 5, 2024
@hjmjohnson hjmjohnson added this to the ITK 6.0 Beta 1 milestone Dec 5, 2024
@hjmjohnson hjmjohnson requested a review from thewtex December 5, 2024 01:52
@hjmjohnson hjmjohnson self-assigned this Dec 5, 2024
@hjmjohnson hjmjohnson force-pushed the update-to-clang-format-19-support branch from 6533a96 to a2f9cb4 Compare December 5, 2024 01:53
@github-actions github-actions bot removed type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Style Style changes: no logic impact (indentation, comments, naming) labels Dec 5, 2024
@hjmjohnson
Copy link
Member Author

FYI: Github actions is having issues: actions/runner#3611

Local macos builds occur properly (my main development environment).

@hjmjohnson
Copy link
Member Author

FYI: Github actions is having issues: actions/runner#3611

Local macos builds occur properly (my main development environment is macos).

@hjmjohnson
Copy link
Member Author

This is precursor work that enables the larger goal in #5013

@hjmjohnson hjmjohnson added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Style Style changes: no logic impact (indentation, comments, naming) labels Dec 5, 2024
@blowekamp
Copy link
Member

The end goal is to have precommit manage the clang format version, and remove our code with build and manages the clang format version?

@hjmjohnson
Copy link
Member Author

The end goal is to have precommit manage the clang format version, and remove our code with build and manages the clang format version?

@blowekamp Yes! The pre-commit version is much easier to manage and covers a wider range of platforms. Using pre-commit will remove much of the complicated cmake code.

I made this PR as a stepping stone to decouple the pre-commit considerations from updating the clang-format version.

dzenanz
dzenanz reviewed Dec 5, 2024
View reviewed changes
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

.clang-format Show resolved Hide resolved
Examples/.clang-format Show resolved Hide resolved
Modules/Core/Common/include/itkMetaProgrammingLibrary.h Show resolved Hide resolved
Modules/Core/Common/test/itkAnatomicalOrientationGTest.cxx Show resolved Hide resolved
Modules/Core/Transform/include/itkBSplineTransform.hxx Show resolved Hide resolved
Modules/Filtering/BinaryMathematicalMorphology/include/itkBinaryErodeImageFilter.hxx Show resolved Hide resolved
Modules/Filtering/DisplacementField/include/itkInvertDisplacementFieldImageFilter.hxx Outdated Show resolved Hide resolved
.clang-format Outdated Show resolved Hide resolved
@hjmjohnson
ENH: update clang-format binaries to 19.1.4
26b683a 
Step 1: Download pypi wheels from https://pypi.org/project/clang-format/#files
for each of the platforms.

Step 2: For each wheel:
```bash
if [ $(uname) == "Darwin" ]; then
  xattr -d com.apple.quarantine ${downloaded_wheel_file}
fi
unzip ${downloaded_wheel_file}
cd clang_format/data/bin
tar -cf clang_format.tar clang-format
```

Step 3: Upload the clang_format.tar file to https://data.kitware.com/
Copy the sha512 for the uploaded tarball and change the
has in Utilities/ClangFormat/DownloadClangFormat.cmake

Step 4: Update the allowed version limits
```cmake
set(CLANG_FORMAT_MIN_VERSION 19.1.4)  # First acceptable version
set(CLANG_FORMAT_MAX_VERSION 20.0)  # First unacceptable version
```
@hjmjohnson
ENH: clang-format wrappers to version 19.1.4
d09f665 
update support scripts to support new clang-format
version.
 Utilities/Hooks/pre-commit-style.bash
 Utilities/Maintenance/clang-format.bash
@hjmjohnson
STYLE: Update .clang-format config version 19.1.4
694457a 
Update .clang-format and Example/.clang-format propogating
current selection forward to the new version.

```bash
cd ${ITK_SOURCE_DIR}
clang-format  --dump-config > /tmp/ITK_clang_format
mv /tmp/ITK_clang_format .clang-format

cd ${ITK_SOURCE_DIR}/Examples
clang-format  --dump-config > /tmp/ITK_Examples_clang_format
mv /tmp/ITK_Examples_clang_format .clang-format
```
@hjmjohnson
STYLE: Rigorous style conformance with clang-format 19.1.4
9bd6b75 
The newer version of clang-format has a more rigorous
enforcement of the ITK style.

Most changes include placing the return value of a function
on it's own line, and moving single line function definitions
to the ITK multi-line format (even in macros).
@hjmjohnson hjmjohnson force-pushed the update-to-clang-format-19-support branch from a2f9cb4 to 9bd6b75 Compare December 5, 2024 23:21
@hjmjohnson hjmjohnson requested a review from dzenanz December 5, 2024 23:21
@github-actions github-actions bot removed type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Style Style changes: no logic impact (indentation, comments, naming) labels Dec 5, 2024
@hjmjohnson
Copy link
Member Author

@dzenanz @blowekamp I have addressed all the issues that you pointed out earlier. I found a setting that minimized the differences with clang version 8 with ~130 fewer changed files.

I'd love to get this PR to make progress toward acceptance because it is a key roadblock for evaluating the next set of build improvements (aka pre-commit).

blowekamp
blowekamp approved these changes Dec 6, 2024
View reviewed changes
Copy link
Member

@blowekamp blowekamp left a comment

Choose a reason for hiding this comment

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

Thank you for moving to a much needed newer version of clang-formatting. It is good to separate the stye preferences from version changes as best we can. Additionally, separating the code formatting changes from the introduction of pre-commit usage is good. Thank you for moving this forwards.

@hjmjohnson hjmjohnson added the type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots label Dec 6, 2024
@hjmjohnson hjmjohnson added the type:Style Style changes: no logic impact (indentation, comments, naming) label Dec 6, 2024
This was referenced Dec 6, 2024
Merged
Open
N-Dekker
N-Dekker reviewed Dec 6, 2024
View reviewed changes
Modules/Core/Common/include/itkAutoPointer.h
Comment on lines -76 to +80
ObjectType * operator->() const { return m_Pointer; }
ObjectType *
operator->() const
{
return m_Pointer;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Interestingly, here it goes the other way (compared to the case of the m_LastPipelineMTime = 0; statement). I like having the return m_Pointer; statement on its own line. 👍

@N-Dekker
Copy link
Contributor

N-Dekker commented Dec 6, 2024

@hjmjohnson Thanks, really great! I'm very glad to see such a major clang-format upgrade!

I'm now going to try your PR locally. Don't know if it's necessary to wait until it works on my Windows laptop...

@hjmjohnson hjmjohnson merged commit 814a24c into InsightSoftwareConsortium:master Dec 6, 2024
17 checks passed
@hjmjohnson hjmjohnson deleted the update-to-clang-format-19-support branch December 6, 2024 13:57
@N-Dekker
Copy link
Contributor

N-Dekker commented Dec 6, 2024

@hjmjohnson Dummy question: I just pulled the master branch and reran CMake. An attempt to commit locally tells me:

clang-format version 19.1.4 is required
Set the path the clang-format 19.1.4 executable with
  git config clangFormat.binary /path/to/clang-format

What should I do? I don't see the clang-format 19.1.4 executable on my disk. My "Bin\ITK\ClangFormat-prefix\src\ClangFormat" still has an old clang-format.exe

@dzenanz
Copy link
Member

dzenanz commented Dec 6, 2024

@N-Dekker From this commit 0b1fabd it can be deduced to get Windows binary from this link.

@N-Dekker N-Dekker mentioned this pull request Dec 17, 2024
Merged
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Dec 19, 2024
@N-Dekker
STYLE: Disable clang-format style option AlignConsecutiveDeclarations
3213446 
No longer enabled the style option of aligning consecutive declarations.

The convention of aligning declarations has always bothered some of us, as
appeared already during the discussion at https://discourse.itk.org/t/update-coding-style-for-itk/2055
(back in 2019).

While this style option may be just a matter of taste, it appears to hinder the
collaborative development process, as it pollutes [git] blame, it makes changes
to lines that aren't really changing, complicating code review, etc., as Sean
McBride remarked at InsightSoftwareConsortium#5015 (comment).
Moreover, it also triggers unnecessary git merge conflicts.

Now that clang-format is recently upgraded for ITK (pull request
InsightSoftwareConsortium#5015 commit
0b1fabd), it appears the right moment to
reconsider this option.
@N-Dekker N-Dekker mentioned this pull request Dec 19, 2024
Closed
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Dec 19, 2024
@N-Dekker
STYLE: Disable clang-format style option AlignConsecutiveDeclarations
4c96943 
No longer enabled the style option of aligning consecutive declarations.

The convention of aligning declarations has always bothered some of us, as
appeared already during the discussion at https://discourse.itk.org/t/update-coding-style-for-itk/2055
(back in 2019).

While this style option may be just a matter of taste, it appears to hinder the
collaborative development process, as it pollutes [git] blame, it makes changes
to lines that aren't really changing, complicating code review, etc., as Sean
McBride remarked at InsightSoftwareConsortium#5015 (comment).
Moreover, it also triggers unnecessary git merge conflicts.

Now that clang-format is recently upgraded for ITK (pull request
InsightSoftwareConsortium#5015 commit
0b1fabd), it appears the right moment to
reconsider this option.
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Dec 20, 2024
@N-Dekker
STYLE: Disable clang-format style option AlignConsecutiveDeclarations
098faec 
No longer enabled the style option of aligning consecutive declarations.

The convention of aligning declarations has always bothered some of us, as
appeared already during the discussion at https://discourse.itk.org/t/update-coding-style-for-itk/2055
(back in 2019).

While this style option may be just a matter of taste, it appears to hinder the
collaborative development process, as it pollutes [git] blame, it makes changes
to lines that aren't really changing, complicating code review, etc., as Sean
McBride remarked at InsightSoftwareConsortium#5015 (comment).
Moreover, it also triggers unnecessary git merge conflicts.

Now that clang-format is recently upgraded for ITK (pull request
InsightSoftwareConsortium#5015 commit
0b1fabd), it appears the right moment to
reconsider this option.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@N-Dekker N-Dekker N-Dekker left review comments

@dzenanz dzenanz dzenanz approved these changes

@blowekamp blowekamp blowekamp approved these changes

@seanm seanm seanm left review comments

@thewtex thewtex Awaiting requested review from thewtex

Assignees

@hjmjohnson hjmjohnson

Labels
type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Style Style changes: no logic impact (indentation, comments, naming)
Projects
None yet
Milestone
ITK 6.0 Beta 1
Development

Successfully merging this pull request may close these issues.

Update clang-format version
5 participants
@hjmjohnson @blowekamp @N-Dekker @dzenanz @seanm