STYLE: Disable clang-format style option AlignConsecutiveDeclarations

[N-Dekker]

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 (@seanm) remarked at #5015 (comment). Moreover, it also triggers unnecessary git merge conflicts.

Now that clang-format is recently upgraded for ITK (pull request #5015 commit 0b1fabd), it appears the right moment to reconsider this option.

[dzenanz]

💯

[dzenanz]

@N-Dekker could you push the changes after applying the new formatting as a second commit? That will facilitate online discussion.

[N-Dekker]

could you push the changes after applying the new formatting as a second commit

Sure! Is there an easy way to re-apply clang-format to all ITK C++ files (not the Python files, CMake files, etc., and not the Third Party files)?

I locally tried the following "hand-made" command-line in Windows, on Git Bash. It does quite a lot, but it may be incomplete:

find . -regex '.*itk[^\\/]*\.\(h\|hxx\|cxx\)' -exec path\\to\\clang-format.exe -style=file -i {} \;

Is there a better way? Or should something like this be good enough?

[hjmjohnson]

I think this should do it:

pixi run pre-commit-run -a clang-format

[N-Dekker]

pixi run pre-commit-run -a clang-format

Thanks @hjmjohnson Really cool, if it works like that.

Unfortunately I got:

me@my-pc MINGW64 /f/X/Src/I/ITK (clang-format-AlignConsecutiveDeclarations-false)
$ pixi run pre-commit-run -a clang-format
Pixi task (pre-commit-run in pre-commit): pre-commit run --all -a clang-format: (Run pre-commit hooks on all repository files)
clang-format.............................................................Failed
- hook id: clang-format
- files were modified by this hook

Do you have a clue?

[hjmjohnson]

@N-Dekker YES! That is the desired behavior. The clang-format failed hook failed because the files were changed by clang format. I think that was the desired behavior. You now need to make a commit of the changed files that represent the new clang-format style.

[N-Dekker]

@N-Dekker YES! That is the desired behavior.

Thanks @hjmjohnson It works 👍

Unfortunately the commit (modifying 3012 files) takes hours, yes literally hours, saying:

check for added large files..............................................Passed
check python ast.....................................(no files to check)Skipped
check for case conflicts.................................................Passed
check illegal windows names..........................(no files to check)Skipped
check json...........................................(no files to check)Skipped

Is there a way to speed up doing such a bulk commit?

[N-Dekker]

The git commit itself took more than 4 hours!!!

[N-Dekker]

https://github.com/InsightSoftwareConsortium/ITK/pull/5096/files says:

The diff you're trying to view is too large. We only load the first 3000 changed files.

Indeed: this pull request would change 3014 files 🤷

If I would do it again, I would prefer to exclude "Examples" from this pull request (and make a separate "Examples" pull request). Also because it just takes too much time to for me to do a single git commit for more than 3000 changed files.

But for now, it's ready for review 🎉

[dzenanz]

Splitting the change into Examples and rest of the toolkit also sounds fine. But good as-is.

[N-Dekker]

Splitting the change into Examples and rest of the toolkit also sounds fine. But good as-is.

Thanks @dzenanz If this PR can get merged "as-is", it's also fine by me. But if it would need to be amended, or if it would get merge conflicts, I'll consider splitting the change into Examples and the rest as well.

[dzenanz]

As we would like consensus on this matter, it is advisable to leave it open for longer to give more chance to all maintainers to pitch in. This will almost certainly lead to conflicts when it comes merge time (just preparing you psychologically 😄).

[hjmjohnson]

The git commit itself took more than 4 hours!!!

Wow! I did this on my SSD drive mac laptop, and applying the changes from the updated .clang-format only took a few seconds.

Seems like perhaps there is something going on beyond just the running of clang-format.

[thewtex]

I do not think this should be changed. It adds a very large readability value. While it may occasionally add some merge conflicts, I do not think that justifies the trade-off. This was the style of ITK before clang-format. With pre-commit clang-format, the spaces are automatically added as-needed, so it is not a burden on the author.

[dzenanz]

I just tried it on my computer, and it took 17 seconds. So it is not Windows-specific, it might be specific to your computer Niels.

[jhlegarreta]

These are my thoughts on this:

• I confess that I have mixed feelings whenever I see clang aligning assignments and changing lines that I have not modified, and reviewing code where this has happened, but I got used to it as I know that it is enforced by the style and what to expect.
• I do not code a lot in C++/ITK-related projects lately, so I cannot measure any potential burden that this is causing; when the decision was made, I got used to it.
• There may be other options such as aligning using the assignment operator, which may be equally controversial.
• I guess we are all aware of this, but whitespace changes can be hidden in GitHub, which may ease inspecting the meaningful changes.
• If the use of false opens the door to the possibility of people aligning things manually using their own criterion, that would be even worse, and put that burden on reviewers, introducing delays (by requiring more time from reviewers asking for the change and from developers making the changes, needing to wait for CIs, etc.).
• Having some criterion (like the current one) is maybe better than not having one.

So I'd vote for keeping the alignment to true.

[N-Dekker]

Thanks very much for your elaborate feedback, @jhlegarreta Just a clarification (or correction?) regarding your last two bullets:

• If the use of false opens the door to the possibility of people aligning things manually using their own criterion, that would be even worse...
• Having some criterion (like the current one) is maybe better than not having one.

I would like to clarify: when disabling AlignConsecutiveDeclarations, the format of declarations remains controlled by clang-format. Disabling this style option still does not allow people to align their declarations manually. Instead it enforces a single space between the type and the variable name, in any variable declaration.

Please let us know if this clarification might make you reconsider your vote 😺

[N-Dekker]

@thewtex Thanks for your feedback! It's obviously a matter of taste, but I find this proposed change an improvement of the readability. Old code (having enabled declaration alignment):

      auto                                     image = VariableLengthVectorImageType::New();
      VariableLengthVectorImageType::IndexType start;
      InternalPixelType                        f(VectorLength);

Proposed with this pull request:

      auto image = VariableLengthVectorImageType::New();
      VariableLengthVectorImageType::IndexType start;
      InternalPixelType f(VectorLength);

[thewtex]

Yes, when there are fewer items, it is more of a preference. This example, with fewer items and highly varying type lengths, is more compact. In general, it is easier to read when they are aligned.

More significantly, when there are many items, it is much easier to identify the types and values when visually aligned.

[thewtex]

me@my-pc MINGW64

@N-Dekker better performance may come from Git Bash (the default with Git for Windows) or the Windows Subsystem for Linux (WSL).

[dzenanz]

That command was fast both in cmd and Git bash.

[jhlegarreta]

I would like to clarify: when disabling AlignConsecutiveDeclarations, the format of declarations remains controlled by clang-format. Disabling this style option still does not allow people to align their declarations manually. Instead it enforces a single space between the type and the variable name, in any variable declaration.

👍 Thanks.

Please let us know if this clarification might make you reconsider your vote

No changes.

[hjmjohnson]

I'm conflicted, my personal (developer) preference is to minimize
git conflicts as much as possible. I "want" this PR for personal
reasons.

However, the ghost of christmas past "aka Bill Lorensen," kept bothering me
during my review of this PR. The reminder was that core ITK developers 'should carry
the extra burden of making end-users or novice users use easier'. Then Luis Ibanez
haunted me to remind me that lesson from "Clean Code" that
'code is read much more than it is written'

I'm converging on the conclusion that my personal preference
is probably not what is best for the community.

I think that when large blocks of code are changed due to this formatting
the decision, that it is often an indication of missing whitespace between blocks
of unrelated variables, or that the variables are declared in a block without
enforcing the principle that variables should be declared in locally to where
they are needed, and in the smallest scope possible.

This is clearly my opinion, but I wanted to give a longer
explanation of my current position.

[N-Dekker]

Thanks for your elaborate reply, Hans. Honestly it's a bit overwelming to me.

Code readability is and has always been one of my main priorities. As well as readability of documentation, commit messages, etc. I hope you don't think otherwise about me!

It's my impression that alignment of declarations sometimes increases readability, and in other cases decreases readability. However, it's partially just a matter of taste. 🤷

A commit like 5b5e4b2 (June 9, 2014) supports your claim that Bill would prefer alignment of consecutive declarations. And apparently, also Luis (@luisibanez) prefers such alignments, looking at "itkTestingExtractSliceImageFilterTest.cxx" commit cba75da (April 13, 2014). However, I don't think that removing such alignments would be against the spirit of ITK. We made many other coding style changes over the years. This would just be one of them.

For the record: The default LLVM style (that you get by clang-format -style=llvm -dump-config > .clang-format) disables AlignConsecutiveDeclarations. The implementation of clang-format (which is itself written in C++) does not do align variable declarations either, looking at https://github.com/llvm/llvm-project/blob/ac5029499561a8f76e71325ffb5c59d92ff84709/clang/lib/Format/AffectedRangeManager.cpp#L24-L27

However, given the current resistance against this pull request I feel that I have to stop my effort now to get it merged. Of course, it's a bit disappointing to me. But then again, I already had my fair share of merged pull requests on ITK, for 2024 😃