STYLE: Enforce ITK style defined by .clang-format by hjmjohnson · Pull Request #1191 · InsightSoftwareConsortium/ITK

hjmjohnson · 2019-08-23T13:15:23Z

ITK/Utilities/Maintenance/clang-format.bash
--clang-format ~/local/llvm/clang+llvm-8.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang-format
--tracked

This patch updates the ITK preferred style to one that can be
automatically enforced, integrated with many IDE's, and based
on externally defined documentation and examples.

See: https://discourse.itk.org/t/update-coding-style-for-itk/2055

Most of the changes make inconsistent coding practices consistent
throughout the toolkit.

One large change is changing the {} indentation style from
Whitesmiths style https://en.wikipedia.org/wiki/Indentation_style#Whitesmiths_style
to an indentation style consistent with many of the other common
indentation styles. The primary reason for this change is to
overcome the common deficiency of IDE's and formatters to support
this uncommon style choice.
Completes the work started in #1094

PR Checklist

🚫 Makes breaking changes
🚫 Makes design changes
🚫 Adds the License notice to new files.
🚫 Adds Python wrapping.
🚫 Adds tests and baseline comparison (quantitative).
🚫 Adds test data.
🚫 Adds Examples to ITKExamples
🚫 Adds Documentation.

gdevenyi · 2019-08-23T13:21:57Z

Is it possible to add a test to new PRs that check for style?

hjmjohnson · 2019-08-23T13:27:58Z

Is it possible to add a test to new PRs that check for style?

That should be possible, and probably not too hard for someone who better understands how to make those checks.

bradking · 2019-08-23T13:29:30Z

@hjmjohnson for the commit that actually applies the mass style change I suggest commiting with --author=... to set the author to a robot user (e.g. kwrobot). That way you won't be git blamed for all the code. For example, see CMake MR 2123 where we updated style there. We also added an empty commit just before the transition and instructions in the commit messages for people to rebase their topics past the formatting changes.

bradking · 2019-08-23T13:31:56Z

Is it possible to add a test to new PRs that check for style?

Yes, the ghostflow-check-master check can be updated to also verify style with clang-format. However, clang-format's format is not quite stable across versions. Therefore we must pick a single major version of clang-format for this purpose. CMake currently uses clang-format-6.0.

gdevenyi · 2019-08-23T13:56:44Z

Also related, is it possible to add this as an (optional) precommit hook for ITK developers?

https://gist.github.com/alexeagle/c8ed91b14a407342d9a8e112b5ac7dab

bradking · 2019-08-23T14:05:36Z

It could be made a pre-commit hook but we'd need to make sure that developers who enable it are using the matching clang-format version.

seanm · 2019-08-23T14:14:25Z

Hopefully a pre-commit hook could be overridden? It would be a shame to discourage small first time patches just because someone doesn't have a specific version of clang-format on their computer.

blowekamp · 2019-08-23T14:21:17Z

Also related, is it possible to add this as an (optional) precommit hook for ITK developers?

https://gist.github.com/alexeagle/c8ed91b14a407342d9a8e112b5ac7dab

I would be more in favor of a CI service than requiring additional tools locally. If it's optional that would be OK tool.

As for version compatibility, VS and Clion come with an embedded version for clang-format, are they close enough for most small changes? It would be nice if the default tool is good enough most of the time for small changes. Needing a specific version for larger changes would be OK.

bradking · 2019-08-23T14:36:47Z

For reference, if we choose a clang-format version other than 6.0 it will take some time to make ghostflow-check-master check it because some infrastructure needs to be upgraded.

hjmjohnson · 2019-08-23T14:49:03Z

@hjmjohnson for the commit that actually applies the mass style change I suggest commiting with --author=... to set the author to a robot user (e.g. kwrobot). That way you won't be git blamed for all the code. For example, see CMake MR 2123 where we updated style there. We also added an empty commit just before the transition and instructions in the commit messages for people to rebase their topics past the formatting changes.

@bradking EXCELLENT IDEAS!!! Thank you. I'll do that.

hjmjohnson · 2019-08-23T14:51:13Z

For reference, if we choose a clang-format version other than 6.0 it will take some time to make ghostflow-check-master check it because some infrastructure needs to be upgraded.

I'm using a modified version of the cmake mechanism with standardization on clang 8.0 (for a new refined C++11 feature support, and because that is the clang-format version that is standard with one of the Visual studio plugins, and CLion plugins.

hjmjohnson · 2019-08-23T14:52:20Z

VS and Clion come with an embedded version for clang-format,

I choose to standardize on clang 8 to be most aligned with those two IDE's :).

N-Dekker · 2019-08-23T15:04:22Z

  const ImageType::IndexType LeftEyeIndex = GetIndexFromMouseClick();
-  ImageType::PointType LeftEyePoint;
-  image->TransformIndexToPhysicalPoint(LeftEyeIndex,LeftEyePoint);
+  ImageType::PointType       LeftEyePoint;


I very much dislike alignment somewhere in the middle of a line of code. It would mean that whenever line 348 is modified, line 349 would also be modified to keep the alignment in sync. Please no!

@N-Dekker, Personally I agree, but this has been discussed several times before, and the historical community agreement is that the benefits in readability outweigh the pain.

FWIW, I agree with @N-Dekker. It will mess up blame & history to re-indent like that.

I agree, it's very hard to maintain this alignment. Even if this was discussed before, since formatting will change now anyway, it would be a good time to re-check what most people prefer.

There are many similar questions where people can have different preferences, so maybe all the controversial decisions could be put up on a public vote. We could give different weight to votes depending on involvement in ITK project (e.g., a core ITK developer could have 10x more vote than an ITK user).

Yes, this is annoying inside functions/methods. Unfortunately, clang-format does not distinguish that from declarations inside classes.

@hjmjohnson Maybe we could completely abolish alignment of declarations because of this?

@dzenanz @lassoan @N-Dekker @thewtex I want to be VERY CLEAR: I am doing my best to provide better automation of style, and not dictate the style. I spent a lot of time finding the set of options that minimizes changes to the current style while providing AUTOMATION.

I'm willing to entertain other global style changes, but to the extent possible I would rather not let the scope of this PR require that every style be re-considered.

Please vote now, and recruit others to support/reject this coding change.

NOTE: Making this change will also require changing KWSTYLE rules, as the current suggestions will cause the KWSTYLE checks to fail.

--- NOW my personal opinion: Let's change KWSTYLE and remove it's enforcement, and change .clang-format AlignConsecutiveDeclarations: true to AlignConsecutiveDeclarations: false

Thank you for taking the time for this, I look forward to not thinking about style anymore. 👔 🕴

Are we going to need KWStyle any more?

My issue with looking through history, is not a single change like this one ( seems to be a very good plan for it ). It is more the frequent white space changes cause by this case and the one below. Thank you for being open to a couple be for the big commit.

kwrobot-v1 · 2019-08-28T22:20:25Z

Errors:

Failed to reserve ref 5c2a0a7 for the merge request: invalid git ref: 'no such commit'.
Failed to run the checks: git error: failed to list commits on the merge request https://github.com/InsightSoftwareConsortium/ITK/pull/1191: fatal: bad object 5c2a0a7df81131c3c0b9f90b06f2256255f6ccf2 .

hjmjohnson · 2019-08-29T10:55:51Z

@thewtex All Green!

Empty macros are nested by clang-format. Explicitly prohibit clang-formatting of lists of macros.

Inconsistent space inside of <> () and [] is resolved to not have spaces consistently as preferred by the ITK community. This was previously unspecified in the style requirements.

BinPackParameters (bool) If false, a function declaration’s or function definition’s parameters will either all be on the same line or will have one line each.

Examples are used to generate Software Guide, and shorter code column limits are needed to render the code into latex.

This is a commit that precedes an automatic application of clang-format-8.0 to update the C++ style of our entire source tree. This may be helpful to rebase a topic branch that was originally based on a commit preceding the transition. One may first rebase the topic on this commit. Then use one of the following approaches. * Rewrite the topic, including this commit, using `git filter-branch` `--tree-filter` with `Utilities/Maintenance/clang-format.bash` to update the style in every commit. Rebase the revised topic, excluding the rewrite of this commit, on the style transition commit. - OR - * Add a `.git/info/grafts` entry to change the parent of the first commit in the topic from this commit to the style transition commit. Rewrite the topic using `git filter-branch --tree-filter` with `Utilities/Maintenance/clang-format.bash` to update the style in every commit. Then remove the graft, which was resolved by the filter. See `git help filter-branch` and `git help repository-layout` for details.

hjmjohnson · 2019-08-29T15:44:52Z

@thewtex @dzenanz I have rebased on the merges from this morning. Last nights builds were clean green from the CI perspective.

This PR required a manual merging of 2 small blocks of conflicts in from the DCMTK merge that occurred this morning.

I tested, and the build succeeded, and all tests pass.

As this has 2 approvals, I'd really like to "rebase and merge" immediately (i.e. not wait 5 hours for another green CI for the small formatting fixes induced by the DCMTK code merges ) so that new PR's can be generated against this rigorously enforced code style.

Please provide a +1 or just rebase and merge at your earliest convenience,
Hans

dzenanz · 2019-08-29T15:56:54Z

In some of the previous builds there was a warning about multi-line comment. Was that also addressed? Not a showstopper if it wasn't. But as there are no other PRs which are ready for merging we could wait a few hours for the CI builds too.

hjmjohnson · 2019-08-29T15:58:48Z

Yes it was addressed last night. A “// clang-format off” “// clang-format on” protection around a comment that had a large latex table imbedded in a comment block. Hans From: "notifications@github.com" <notifications@github.com> Reply-To: InsightSoftwareConsortium/ITK <reply@reply.github.com> Date: Thursday, August 29, 2019 at 10:56 AM To: "nipype@noreply.github.com" <ITK@noreply.github.com> Cc: Hans Johnson <hans.j.johnson@gmail.com>, Mention <mention@noreply.github.com> Subject: Re: [InsightSoftwareConsortium/ITK] STYLE: Enforce ITK style defined by .clang-format (#1191) In some of the previous builds there was a warning about multi-line comment. Was that also addressed? Not a showstopper if it wasn't. But as there are no other PRs which are ready for merging we could wait a few hours for the CI builds too. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

./Utilities/Maintenance/clang-format.bash \ --clang-format ~/local/llvm/clang+llvm-8.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang-format \ --tracked This patch updates the ITK preferred style to one that can be automatically enforced, integrated with many IDE's, and based on externally defined documentation and examples. See: https://discourse.itk.org/t/update-coding-style-for-itk/2055 Most of the changes make inconsistent coding practices consistent throughout the toolkit. Community discussions regarding changes to the current defined coding style are explicitly not included in this request so that they can be independantly reviewed and considered. One large change is changing the {} indentation style from `Whitesmiths` style https://en.wikipedia.org/wiki/Indentation_style#Whitesmiths_style to an indentation style consistent with many of the other common indentation styles. The primary reason for this change is to overcome the common deficiency of IDE's and formatters to support this uncommon style choice. Many ITK developers provided insightful comments and suggestions that resulted in this set of style comprimises. Co-authored-by: Hans Johnson <hans-johnson@uiowa.edu> Co-authored-by: Dzenan Zukic <dzenan.zukic@kitware.com> Co-authored-by: Brad King <brad.king@kitware.com>

dzenanz

Merging this today seems plausible to me.

dzenanz · 2019-08-29T16:18:25Z

+  ParallelizeArray(SizeValueType             firstIndex,
+                   SizeValueType             lastIndexPlus1,
+                   ArrayThreadingFunctorType aFunc,
+                   ProcessObject *           filter);


I thought we agreed on no declaration alignment and left-aligned pointers?
https://doodle.com/poll/sb4aqtpsy6qs7z3h
https://doodle.com/poll/yn8bxxtdzz97xg38

While pointer alignment is more of a personal choice, declaration alignment is not quite common outside of ITK. That's why people less involved with ITK seem to prefer no alignment.

@dzenanz Please talk to @thewtex.

I was under the opposite impression that this PR was to use the current alignment strategies (i.e. smaller overall differences from current style), and that further discussions were needed for both of those poll items. The further discussions will need to be separate pull requests, and each will need several weeks of a community discussion on discourse.

Discussions from PR InsightSoftwareConsortium#1191 describing community desire to change enforcment of style that have proven to be disruptive or difficult to maintain.

Applies the clang format defined by InsightSoftwareConsortium/ITK#1191.

Removed spaces between class and member names of IO classes, that were accidentally introduced with pull request InsightSoftwareConsortium#1191 commit 2074c2f "STYLE: Enforce ITK style defined by .clang-format". Follow-up to pull request InsightSoftwareConsortium#2096 commit 734d6f8 "STYLE: Remove space between class and member names in C++ source files"

Removed spaces between class and member names, that were accidentally introduced with pull request InsightSoftwareConsortium#1191 commit 2074c2f "STYLE: Enforce ITK style defined by .clang-format". Using bash commands like: find . $ -iname *.cxx $ -exec perl -pi -w -e 's/([A-Z]\w+) \:\:(\w)/$1::$2/g;' {} \; Follow-up to pull request InsightSoftwareConsortium#2096 commit 734d6f8 "STYLE: Remove space between class and member names in C++ source files"

Removed spaces between class and member names, that were accidentally introduced with pull request #1191 commit 2074c2f "STYLE: Enforce ITK style defined by .clang-format". Using bash commands like: find . $ -iname *.cxx $ -exec perl -pi -w -e 's/([A-Z]\w+) \:\:(\w)/$1::$2/g;' {} \; Follow-up to pull request #2096 commit 734d6f8 "STYLE: Remove space between class and member names in C++ source files"

Removed spaces between class and member names, that were accidentally introduced with pull request InsightSoftwareConsortium#1191 commit 38e2d2d "STYLE: Enforce ITK style defined by .clang-format". Using bash commands like: find . $ -iname *.cxx $ -exec perl -pi -w -e 's/([A-Z]\w+) \:\:(\w)/$1::$2/g;' {} \; Follow-up to pull request InsightSoftwareConsortium#2096 commit 4b95bbd "STYLE: Remove space between class and member names in C++ source files"

hjmjohnson requested a review from thewtex August 23, 2019 13:15

hjmjohnson self-assigned this Aug 23, 2019

hjmjohnson requested review from a team, blowekamp, dzenanz, jhlegarreta and zivy August 23, 2019 13:15

hjmjohnson added the type:Style Style changes: no logic impact (indentation, comments, naming) label Aug 23, 2019

hjmjohnson modified the milestones: ITK v5.0.2, ITK v5.1b01 Aug 23, 2019

hjmjohnson changed the title ~~STYLE: Enforce ITK sttyle defined by .clang-format~~ STYLE: Enforce ITK style defined by .clang-format Aug 23, 2019

N-Dekker reviewed Aug 23, 2019

View reviewed changes

Comment thread Examples/DataRepresentation/Containers/TreeContainer.cxx Outdated

N-Dekker reviewed Aug 23, 2019

View reviewed changes

Comment thread Examples/DataRepresentation/Containers/TreeContainer.cxx Outdated

N-Dekker reviewed Aug 23, 2019

View reviewed changes

Comment thread Examples/DataRepresentation/Containers/TreeContainer.cxx Outdated

N-Dekker reviewed Aug 23, 2019

View reviewed changes

Comment thread Examples/DataRepresentation/Mesh/AutomaticMesh.cxx Outdated

hjmjohnson force-pushed the apply-new-clang-format-style branch from 5c2a0a7 to 1774128 Compare August 29, 2019 01:56

hjmjohnson added 6 commits August 29, 2019 10:20

COMP: Change KWStyle to allow reformat to pass.

049b4d0

COMP: Fix clang-format nesting of empty macros

242ef8d

Empty macros are nested by clang-format. Explicitly prohibit clang-formatting of lists of macros.

STYLE: No-space inside <> () []

0101b25

Inconsistent space inside of <> () and [] is resolved to not have spaces consistently as preferred by the ITK community. This was previously unspecified in the style requirements.

STYLE: Change the packing of parameters.

5de4e7f

BinPackParameters (bool) If false, a function declaration’s or function definition’s parameters will either all be on the same line or will have one line each.

STYLE: Use shorter column length for Software Guide.

6933fd8

Examples are used to generate Software Guide, and shorter code column limits are needed to render the code into latex.

hjmjohnson force-pushed the apply-new-clang-format-style branch from 1774128 to 2b0cf47 Compare August 29, 2019 15:37

dzenanz mentioned this pull request Aug 29, 2019

ENH: Streaming-equivalence GTest for GaussianInterpolateImageFunction #1012

Merged

hjmjohnson and others added 2 commits August 29, 2019 11:03

STYLE: Fix spelling error foramt -> format

01e8de2

hjmjohnson force-pushed the apply-new-clang-format-style branch from 2b0cf47 to f3faab0 Compare August 29, 2019 16:03

dzenanz reviewed Aug 29, 2019

View reviewed changes

hjmjohnson merged commit 2074c2f into InsightSoftwareConsortium:master Aug 29, 2019

dzenanz mentioned this pull request Sep 25, 2019

Documentation should reflect new clang-format enforced style #1270

Closed

jhlegarreta mentioned this pull request Sep 25, 2019

Update the Code Style Guide section to match the .clang-format tool decisions InsightSoftwareConsortium/ITKSoftwareGuide#133

Closed

hjmjohnson deleted the apply-new-clang-format-style branch October 23, 2019 13:30

SimonRit mentioned this pull request Dec 3, 2019

Clang.format RTKConsortium/RTK#275

Merged

SimonRit added a commit to RTKConsortium/RTK that referenced this pull request Dec 4, 2019

Merge pull request #275 from SimonRit/clang.format

87448ab

Applies the clang format defined by InsightSoftwareConsortium/ITK#1191.

N-Dekker mentioned this pull request Sep 26, 2022

STYLE: Remove space between class and member names (follow-up) #3664

Merged

Uh oh!

Conversation

hjmjohnson commented Aug 23, 2019

PR Checklist

Uh oh!

gdevenyi commented Aug 23, 2019

Uh oh!

hjmjohnson commented Aug 23, 2019

Uh oh!

bradking commented Aug 23, 2019

Uh oh!

bradking commented Aug 23, 2019

Uh oh!

gdevenyi commented Aug 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bradking commented Aug 23, 2019

Uh oh!

seanm commented Aug 23, 2019

Uh oh!

blowekamp commented Aug 23, 2019

Uh oh!

bradking commented Aug 23, 2019

Uh oh!

hjmjohnson commented Aug 23, 2019

Uh oh!

hjmjohnson commented Aug 23, 2019

Uh oh!

hjmjohnson commented Aug 23, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kwrobot-v1 Bot commented Aug 28, 2019

Uh oh!

hjmjohnson commented Aug 29, 2019

Uh oh!

hjmjohnson commented Aug 29, 2019

Uh oh!

dzenanz commented Aug 29, 2019

Uh oh!

hjmjohnson commented Aug 29, 2019 via email

Uh oh!

dzenanz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

gdevenyi commented Aug 23, 2019 •

edited

Loading