[llvm-dev] [RFC] Semi-Automatic clang-format of files with low frequency

[Chris Tetreault via llvm-dev]

> My comment there was more targeted against those who would take your idea and push it to unnecessary and counterproductive extremes.

There's nothing counterproductive about just formatting everything and being done with it. The goal of doing clang-format-diff as we commit code is to minimize churn so that there isn't one big patch that conflicts with everything. But if you only format the lines that get touched by a commit, the work will never be done. At some point you need to finish the work, or the tool might as well be called clang-make-the-code-inconsistent.

>Since you're interested in thoughts about clang-format's capabilities, I agree with Matt that the strictness of the approach is a weakness that can frequently make code *less* readable. In addition to what he mentions, here's an example of a bad change that clang-format wants to make that I found after half a minute of scanning our AMDGPU backend
>code:
>
>   // We only support LOAD/STORE and vector manipulation ops for vectors
>   // with > 4 elements.
>-  for (MVT VT : { MVT::v8i32, MVT::v8f32, MVT::v16i32, MVT::v16f32,
>-                  MVT::v2i64, MVT::v2f64, MVT::v4i16, MVT::v4f16,
>-                  MVT::v4i64, MVT::v4f64, MVT::v8i64, MVT::v8f64,
>-                  MVT::v16i64, MVT::v16f64, MVT::v32i32, MVT::v32f32 }) {
>+  for (MVT VT :
>+       {MVT::v8i32, MVT::v8f32, MVT::v16i32, MVT::v16f32, MVT::v2i64,
>+        MVT::v2f64, MVT::v4i16, MVT::v4f16, MVT::v4i64, MVT::v4f64, MVT::v8i64,
>+        MVT::v8f64, MVT::v16i64, MVT::v16f64, MVT::v32i32,
>+ MVT::v32f32}) {
>     for (unsigned Op = 0; Op < ISD::BUILTIN_OP_END; ++Op) {
>       switch (Op) {
>
>I don't particularly mind that clang-format puts that initializer list onto a new line, or that it changes the whitespace around braces. What I do mind: the original code lays the initializer list out carefully such that integer and floating point types always come in pairs on the same line: v8[if]32 and v16[if]32 on the first line, v2[if]64 and
>v4[if]64 on the second line, and so on. clang-format mindlessly mushes the initializer list together in a way that makes it harder to see at a glance what is going on.
>
>(Note that this isn't code that I wrote, so I'm not emotionally attached to it or anything. But I've run into this kind of problem many times during development.)
>
>I believe the common theme here is that clang-format ought to have a mode in which it is more accepting of different layouts of lists co-existing within the same code base. If that feature was available, I would be a strong proponent for adopting it in LLVM itself.
>
>Cheers,
>Nicolai

The problem is that while you may find the way it's formatted to be easier to read, not everybody does. If I were formatting it manually, I probably would have had 8 lines of pairs, with one pair per line, or I would have had all the integer types then all the float types if the order is not important. Now we have a disagreement, and a needless code review issue in the future when somebody adds a new MVT that needs to be handled and fails to figure out the pattern. That's the point of the tool. It can't be reasoned with. It can't be bargained with. It doesn't feel pity of remorse or fear. And it absolutely will not stop. Ever. Until your code is formatted correctly.

Arguing about code format in code review is a productivity drain. Time spent figuring out the style of this one random file is a productivity drain. Time spent inserting a bunch of spaces in a 43 line switch because you added a new case that is just 1 character too long is a productivity drain. We have a tool that can do this for us. Maybe it doesn't format everything exactly how we'd like, but at least you won't have to justify it in code review. Sure, maybe it will miss things, but at least you don't have to push a new patch because a line is too long or you put the * next to the type rather than the variable name. The tool will evolve and improve. Frankly, I'm surprised that it doesn't already perfectly implement the LLVM coding style, but surely this is the goal.

From your previous email:

> Hard no on the shorter period. I have changes I'm working on for several months at a time. 1 year is a minimum for this in my opinion.

Your patch should already be formatted using clang-format-diff, right? Phabricator is configured to complain on your commit if this is not the case, so I don't see how a clang-format commit could possibly cause a conflict for you.