Update Coding Style for ITK

Engineering
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” (GitHub - psf/black: The uncompromising Python code formatter) 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

13

Now that the format is enforced automatically by clang-format, does that mean that rerunning clang-format locally on an ITK clone should have no effect at all?

I did see some changes locally, but maybe it’s because I have a different version of clang installed locally.

14

Yes, it should have no effect – we run it on every pull request and merge to ensure this is the case :green_salad: :robot:. But the same version of clang-format and the configuration file are required.

15

Thanks Matt. I just tried clang-format version 11.0.0 inside VS2019 Clang Power Tools 6.3.0, and I was surprised to see many changes. For example, in https://github.com/InsightSoftwareConsortium/ITK/blob/v5.1.1/Modules/Core/Common/include/itkColorTable.hxx#L304

  case 'r':
  {
    return m_Color[c][0];
  }

Became (locally on my machine):

  case 'r': {
    return m_Color[c][0];
  }

I guess I should try an older version of LLVM, right?

16

Yes, the version used is 8.0, which can be downloaded here:

https://data.kitware.com/#collection/57b5c9e58d777f126827f5a1/folder/5d635cbcd35580e6dcbd80b6

However, if ITK is built and the development setup script is executed, this will be downloaded for you and executed on git commit.

2 Likes
17

@matt.mccormick @hjmjohnson Are there any plans to upgrade clang-format any time soon? Just a question, not a request :smile:

I’m asking specifically because we consider moving elastix to clang-format as well. https://github.com/SuperElastix/elastix/pull/336

18

Niels,

I don’t think so. It is a fairly big undertaking to move to a new version of clang-format, or to change the default version.

It would require a champion willing to do most of the work to address and justify the need for updating all of ITK, SimpleITK, and the remote modules.

Hans

1 Like
19

I noticed that Clang-Format sometimes puts a space in between ClassName and ::MemberName, as DataObject ::Update at

But on many other places, it does not put a space there, for example CStyleCommand::SetClientData at

Is that a Clang-Format bug? @hjmjohnson what is the preferred ITK style?

20

Niels,

My point of view is that the approved style is defined in the way that clang-format version 8 applied the settings defined in the ITK/.clang-format. If clang-format version 8 has a bug in it, then that bug becomes part of the style.

The emphasis is that an automation to keep the code clean and consistent takes precidence over a long list of manually enforced rules. Automation is key.

This seems to be an anomaly in the formatting with most of the code NOT having a space. I suppose you could use https://stackoverflow.com/a/25642688

// clang-format off
...
// clang-format on

to fix this clang-format anomaly.

1 Like
21

That is possible, but this is way more distracting than an extra space. clang-format annoys me sometimes too, but it is better than manually maintaining style.