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

Tue Jun 30 19:03:18 PDT 2020

On 6/30/20 5:03 PM, Chris Tetreault via llvm-dev wrote:
>> 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.

I agree. But I think that we should view this as an iterative process. 
We should generate patches for review, and realize that we might need to 
update clang-format in some cases instead of just always updating the 
code. clang-format has evolved over time, and I doubt that evolution has 
stopped now.

>
>> 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.

Okay, wait. Let's not solve problems that we don't have. I don't ever 
recall such a debate over formatting in a review thread (and while I 
don't any longer, for years, I read every review on the mailing list). I 
agree that your formatting suggestion is better than the current one, 
but if both the current formatting and your suggested improvement are 
better than what clang-format provides, so using the clang-format 
version seems like a regression. We should improve things so that using 
clang-format would be an improvement, or we should figure out how to 
exempt this part of the code.

>
> 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.

No one I know likes spending their time formatting code. We do it when 
we feel it is important. Code is read more than it is written, and 
optimizing to make the code easier to read is very important. Formatting 
lists of things, etc. nicely has helped me catch bugs in the past, and 
even if the price for that is having to comment in a review thread about 
the formatting, or spend some time formatting, I'll take it. When things 
are nicely arranged, it's sometimes more obvious when something just 
isn't right. Tracking down bugs in compilers takes far more time than 
all of that.

Thanks again,

Hal

>
> 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.
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory