Unify code formating with clang-format?

[D4N]

I have been using clang-format (an automated C/C++/Objective C code formatter) for personal projects and find it very convenient to use, as it ensures that everything is formatted uniformly. It has a lot of options to tweak, so the resulting look can be highly customized (the available options can be found here or seen in action with this handy webpage) but one can also go with one of the presets.

How about we use it for exiv2, too?

[clanmills]

Let me be sure that I understand what this is. Is this a tool to totally reformat the source code of Exiv2?

I'd like to see this in action on the code base before deciding to adopt this. I assume it will cause a lot of changes to the code layout. Remember that OpenHub monitor our code and I don't want v0.26.1 to contain an "alarming" number of lines of code being changed between v0.26 and v0.26.1.

Perhaps this should be deferred for v0.27 when I believe we're all coming to agree that the major feature of v0.27 is to "modernise the code".

[D4N]

Let me be sure that I understand what this is. Is this a tool to totally reformat the source code of Exiv2?

Essentially yes, although the focus of a clang-format is to ensure a consistent style without the programmers having to actually format the code themselves.

Also, it can be tweaked quite a lot, so as long as the Exiv2 source code is formated consistently, the impact shouldn't be that big. In practice, there will be of course a huge diff as probably no one will go through the hassle of finding the "perfect" configuration.

Deferring this to 0.27 is a sensible choice, if you want to avoid a huge commit. Otherwise we could also start using it gradually, as clang-format can also format only a region of code (at least Emacs' clang-format mode can do that).

There are some big projects using clang-format, for instance Chromium and Mozilla (they have their own builtin styles beside LLVM, WebKit and Google).

[clanmills]

I think it's a good idea to have a consistent style. The only way to achieve that is to use tools and something based on clang is unlikely to introduce bugs in the C++ code.

It's not the single "huge commit" that bothers me, it's the metric of "lines of code changed = 100%" that bothers me for a "dot" release.

When we start on v0.27, we might even have several "huge commits" if we choose to change our "style template" a couple of times.

What about patches after we've done this. That could create a challenge. That's a strong reason to avoid a "huge commit".

If Pascal is cherry-picking changes on behalf of the distros, we should discuss this with him. Let's do nothing about this at present and discuss it at a team meeting later in the year. We should invite Pascal to the meeting and hear his thoughts.

I wonder if there's anyway to work with "old" and "reformatted" code side by side. We could continue to develop the "old" code, however the published code as been "reformatted". Or that might be a pointless nightmare.

[D4N]

Oh, yeah, cherry picking commits after a huge reformat is going to be ridiculously hard. So maybe let's only do this, if we can find a clang-format config with a small impact.

[piponazo]

I also use clang-format in most of my projects and it is very easy to configure and to apply to all the c++ files. We can even do some fancy things once the full project is reformatted, like: adding a CI job forr checking with travis-ci that the format does not differ from our defined style :).

For sure, I also think it is a good idea to wait until 0.27 for this huge change.

[clanmills]

I think we should approach this with considerable caution. Remember that Exiv2 is open source. It's one thing in the office to reformat code and take the consequences on our shoulders. However we don't know the consequences on our users. In v0.26.1 we could announce that we are considering this for a future release and invite comment. It would be helpful to learn from others who have travelled this road.

Perhaps the v0.27 project should continue to develop the existing code and publish a branch of machine generated reformatted code with the declared intention that v0.28 will be "all new code".

[D4N]

I think deferring it for now is probably the smartest, but I would nevertheless like to point out some issues that I overlooked and that might cause some trouble. As Robin said, the diff is probably going to be huge due to the reformat (unless we find the "perfect" config). This will make backporting patches a really big pain. And I have myself noticed few years ago that different versions of clang-format tended to occasionally produce differently formatted code (I think it was mostly with lambdas, which were kind-of new back then, so the issue might be gone by now). Thus, enforcing a clang-format clean code could actually lead to unwanted CI failures due to the CI using a different version that the patch submitter. And also to unnecessary "noisy" diffs when code gets reformated over and over again (one can work around that with git by only staging certain parts of a file for committing, but its just more work somewhere else again). I think it would be best if we experiment with it a bit in v0.27 and then decide if it helps the project or causes more trouble. Robin Mills <notifications@github.com> writes:

…

I think we should approach this with considerable caution. Remember that Exiv2 is **open source**. It's one thing in the office to reformat code and take the consequences on our shoulders. However we don't know the consequences on our users. In v0.26.1 we could announce that we are considering this for a future release and invite comment. It would be helpful to learn from others who have travelled this road. Perhaps the v0.27 project should continue to develop the existing code and publish a branch of machine generated reformatted code with the declared intention that v0.28 will be "all new code". -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #106 (comment)

[D4N]

This has been addressed by #152. I would close this issue then.