Update Coding Style for ITK

#1

Reference implementation to review/discuss

Background

This is primarily the start of a long process to evaluate how to simplify the
burden of code formatting styles. The goal is to have at least one tool that can automatically
enforce the desired style.

Benefits of choosing clang-format is that the tool:

Considerations to be aware of

  • the whitesmith style of indentation is not supported by clang-format or any of the common style guides used by recent open source projects. This implies that the bracket indentations will need to change (similar to VTK’s change a few years ago).
  • clang-format parses the code (making the tool robust), but then generates the formatted code from the parsed representation. This means that the formatting is for the entire rule set, not just one rule at a time.


Rational

I have recently started to convert to using automated strict formatting conventions, ONLY IF THE STRICT RULES can be fully automated. A flexible choice that requires manual decisions, in my opinion, is less effective than strict rules that can be widely automated.

I have been partially converted based on the doctrine espoused by the python “Black” (https://github.com/python/black) formatting tool. Since submitting to “Black” as the authority for formatting in python, and allowing my editors to auto enforce that style, I find that I am saving a lot of time doing house cleaning. I can write python code as a stream-of-algorithmic-conciousness, press [File][Save], and Black reformats the code as needed for style and future readability. I LOVE IT!

Now… The trick is finding tools, and providing documentation, that can be widely automated. CLion, Visual studio, vim, emacs all have plugins that automate compliance by relying on clang-format. I’m working on that this summer. I think I have a “clang-format” based solution that will be no more difficult than the “kwstyle” tests.

5 Likes
#2

This is much needed!

the whitesmith style of indentation

This is the weirdest choice in all of ITK’s style guide. Virtually no tool supports it. I dislike it because of lack of support from tools. I will not regret sacrificing it.

Having a style and a tool to automatically apply that style will relieve us of the burden of thinking about it. It is the way to go!

#3

While I’m interested in such automatic formatting tools, I’m always afraid that it might take away too much of freedom of developers, trying to express their intention. For example:

y = a*x + b

Might be reformatted automatically to

y = a * x + b

While the former might have expressed the meaning more clearly.

Having said this, I would find it very useful to let clang-format do the indentation, as some of the projects I’m involved with use tabs, others use four spaces, and yet some other project uses two spaces :wink:

Thanks so far, @hjmjohnson

#4

+1

QtCreator has a clang-format plugin that will format your code on save in 1 of 2 ways. Either just the code that you edited or the entire file. Your choice.

If you really want to disable clang-format for a section then use the comments:

// clang-format off

// clang-format on

I have a few sections of my code that I have specially formatted that I don’t want clang-format to touch so I just wrap them in the comments.

Unfortunately C++ doesn’t really have a standard style but moving ITK to one of the base styles would be great, or just having a .clang-format file in the repo would help those of us with editors that utilize clang-format. The time gained back from simply “formatting the code to adhere to a projects” style is wonderful.

2 Likes
#5

I’m still traumatized because early on @blowekamp said my code was unreadable because I hadn’t properly used whitesmith style. :sob:

#6

I installed the clang-format plugin for VSCode a few months ago, and haven’t looked back. It saves so much time.

By coincidence, I haven’t had to touch ITK code in that timeframe, so I hadn’t yet noticed that ITK doesn’t have a .clang-format file :wink:

#7

ITK does have a .clang-format file, but it doesn’t do block indentation properly, which is very annoying.

1 Like
#8

With commit STYLE: Enforce ITK style defined by .clang-format, the formatting of the function body of ImageRegionRange::begin() has changed from:

Before the commit:

return iterator
{
  m_BufferBegin + Self::ComputeOffset(m_OffsetTable, m_BufferedRegionIndex, m_IterationRegionIndex),
  m_OffsetTable,
  OffsetType(),
  m_IterationRegionSize
};

To:

After the commit:

return iterator{ m_BufferBegin + Self::ComputeOffset(m_OffsetTable, m_BufferedRegionIndex, endRegionIndex),
                 m_OffsetTable,
                 iterationOffset,
                 m_IterationRegionSize };

See https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/Core/Common/include/itkImageRegionRange.h#L377

Was this change intended? Personally I would prefer to have a pair of matching curly braces { and } on either the same line of code, or the same column (one exactly below the other). As it was before the commit.

#9

Personally, I am most bothered by local variable declaration alignment. But clang-format is not super-configurable (yet), so we have to live with some of the default weirdness which we can’t fine-tune yet.

And there will always be some disagreement about ideal code style, but automated formatting and consistency it brings outweighs having humans bother with style.

1 Like
#10

If the formatting really goes off the rails (which I have seen a few times in our own projects) you can always tell clang-format to ignore a small section of code with //clang-format off and //clang-format on

2 Likes
#11

Thanks, Dženan. Honestly, I’m bothered by any alignment that depends on the length of an identifier on another line of code. I feel very much supported by the talk Seven Ineffective Coding Habits of Many Programmers by Kevlin Henney. He cares very much about the visual aspects of source code, and argues in favor of a formatting style that is “invariant and unchanging under refactoring”.

However, is code alignment still open for discussion? @matt.mccormick wrote at https://github.com/InsightSoftwareConsortium/ITK/pull/1191#issuecomment-524887830

Changes to the style should be discussed with the community, including a thread on Discourse that is available for a few weeks so everyone has a chance to participate, that are justified for reasons beyond personal preference.

That certainly makes sense to me. But it seems hard to reach a consensus on this issue…

PS For the record: Here is the related poll (by Dženan), “Align consecutive declarations?”: https://doodle.com/poll/sb4aqtpsy6qs7z3h