Code style conformance

[hjmjohnson]

This pull request provides consistent code style conformance for all the code in vxl. It uses tools similar to those used by ITK to accomplish the same task.

[hjmjohnson]

@thewtex @DVigneault @dstoup

I would love to hear your comments about how we can make the code more uniform. I have seen that other bug/warning fixes are often confounded when bug/warning fixes are unnecessarily confounded with code style consistencey changes.

By having a consistent style, it makes reading all the code easier, and it makes future patch sets easier to review.

[hjmjohnson]

This is a style choice. It can be turned off. Many places would indent the contents of if/else pragma statements.

Some code had it each way. I went with indenting to make it easier to identify code blocks of pramas.

[sudomakeinstall]

@hjmjohnson Brilliant!! This is far overdue and will make working with this code dramatically less painful. Ignore my inline comments: I didn't notice that the indented defines and pragmas were nested and thought that the script was doing something funky.

[hjmjohnson]

bsta_algo_test_fit_gaussian <-- failed on clang
gevd_test_noise <-- failed on gcc

Both of these tests pass on my computer. I think these are transient failures not related to this patch set.

[hjmjohnson]

@bradking @rfabbri @seanm Do you have objections to this change?

[bradking]

I'm fine with the change, but I have a couple procedural comments:

• Large commits changing thousands of files at once are harder to deal with than smaller commits. We could consider running the script on one module at a time and committing each separately.
• Please do not create topic branches in the main repository. That causes confusion over which branches are official upstream integration branches and which are not. Anyone using the Fork button will get the extra branches that happen to be around when they fork. Instead push the branch to your own fork and submit the PR from there.

[bradking]

Actually, if we are going to go through the trouble of a sweeping code style update, perhaps we should first discuss whether to update the indentation style. The current style with content aligned with curly braces is called Whitesmiths style. While it was popular when vxl, ITK, and VTK started it is much less so now and few editors support it. This was discussed on the VTK dev list and after many alternative styles were debated the conclusion was that the least disruptive change is to just move all the curly braces two spaces to the left. That modernizes the content (non-)alignment without making any other style changes.

[hjmjohnson]

@bradking Great Comments. I had mostly expected this patch set to be rejected as is, and a second (and perhaps third) iteration before we get some (not unanimous) agreement.

There are many tools that have defaults for the "Google C++ style" or the "LLVM" style.

In my mind being consistent is the most important. Having support for maintaining that consistency is second most important. By choosing an established style protocol gives the advantage of offloading the maintenance of the style documentation and formatting tool (both editors and code beautifiers) support to a larger community that spans many projects.

Here's my current plan (please suggest a different approach):

1. I'll reformat the whole codebase to "Google C++ Style" (ensure compiles and no new test failures)
2. See if people love/hate the changes
3. Re-do code changes one module at a time.

Note: I don't have a strong preference on most style issues, and the "Google C++ Style" is just the most prominant (perhaps not best) one that I could find.

[sudomakeinstall]

@bradking I'm not religiously attached to any style method either, but I do value consistency between vxl and ITK (and VTK, though I'm less familiar). I don't know if the discussions you've referred to have resulted in any official changes to the guidelines, but the style guides I've been using (ITK [1], VTK [2]) both suggest that the curly braces be done the way @hjmjohnson suggested originally. If there's will to change practice towards something like the Google guide, I would have no problem, but for the time being I wonder if maintaining multiple practices would be confusing. Do we have a good sense of who the main users of vxl are? From the github activity it feels like the majority of vxl users are also using ITK/VTK, in which case consistency is in my mind a bigger issue, but I could be wrong.

[1] http://www.vtk.org/Wiki/ITK/Coding_Style_Guide#Use_of_Braces
[2] https://docs.google.com/document/d/1nzinw-dR5JQRNi_gb8qwLL5PnkGMK2FETlQGLr10tZw/edit

[hjmjohnson]

To be really clear here: I am ONLY advocating for consistentcy. I will take community comments and do by best to reconcile differences of opinions.

[bradking]

A few more comments:

• I've looked at clang-format quite a bit. I like that one places a .clang-format in the top of the source tree. Editor plugins can find it and actually run clang-format optionally. It also has a BasedOnStyle option with a few common styles. By choosing one we end up with a .clang-format file that is very small.
• For reference, the VTK discussion I mentioned is here. Agreement was reached on just moving the curlys and nothing else but no one followed up with actually implementing the change.
• IIRC the policy in vxl has only ever been to stay consistent within each individual file, so there could be many styles in use across vxl.

[bradking]

If we are to change styles and achieve consistency between ITK and VXL then this should probably be brought up on the ITK list first since that has more interested parties. Since this PR is also based on a topic branch in the main repository I propose we close this PR, delete the branch, and move to a new one in @hjmjohnson's fork.

[hjmjohnson]

@bradking Sorry about using the main branch for the pull request. Future requests will be made from a forked version.

This is a big decision, and will probably take more than a few days to get rigtht. I'll pull all the comments from this and other threads together this weekend, and make proposals to the ITK community.

I also like the clang-format model. It seems like it will integrate more naturally with editors automated cleanings.

With respect to ITK, it also means some KWSTYLE changes.

[thewtex]

+1 to all the comments.

I think the most effective proposed style change is one that provides a
.clang-format file, an automated way to run clang-format against the code
base in a test, an automated way that downloads the clang-format binary for
developers, and a clang-format git hook that runs on commits.

On Fri, Jan 29, 2016 at 9:28 AM, Hans Johnson [email protected]
wrote:

Closed #145 #145.

—
Reply to this email directly or view it on GitHub
#145 (comment).