Binaryen: clang-format revisited + line length of 80 cols

[aheejin]

tl;dr: I suggest we apply clang-format to the codebase and make line length wrap to 80 cols.

Background

We added .clang-format in #1407 and tried to format things according to it in #1435, but #1435 had some controversial changes and didn't get enough support, so it was closed.

The problems of #1435 and my suggestions are:

• We tried to apply clang-tidy together, and we had some new errors. I think applying clang-tidy can be done in separate PRs if we are gonna do it, and that's not really related to the formatting issue.

• Sometimes clang-format hurts readability. For example, in src/asmjs/shared-constants.cpp, clang-format changed this code

cashew::IString GLOBAL("global"),                                                
                NAN_("NaN"),                                                     
                INFINITY_("Infinity"),                                           
                NAN__("nan"),                                                    
                INFINITY__("infinity"),                                          
                TOPMOST("topmost"),                                              
                INT8ARRAY("Int8Array"),  
...

to this:

cashew::IString GLOBAL("global"), NAN_("NaN"), INFINITY_("Infinity"), NAN__("nan"),
  INFINITY__("infinity"), TOPMOST("topmost"), INT8ARRAY("Int8Array"), INT16ARRAY("Int16Array"),
  INT32ARRAY("Int32Array"), UINT8ARRAY("Uint8Array"), UINT16ARRAY("Uint16Array"),
...

And apparently there was no way to preserve this kind formatting in clang-format. But I think we can exempt this kind of code from clang-format; applying clang-format does not necessarily mean every single file should conform to it.

• It disabled one-line if or case statement formatting. In many parts of our codebase, we have one-line if or case statements like this:

if (condition) something;

switch (var) {
case 1: some action; break;
case 2: other action; break;
...
}

But our .clang-format setting didn't allow this. Of course clang-format has options to allow it, but the problem was, they _forced_ every if and case statements that are within line length limit (which was 100 cols) to be a single line, which was a rather big change we didn't want. Basically we had to choose between having no single if/case vs. merging every if/case into a single line as long as it doesn't exceed 100 cols. This issue was discussed in https://github.com/WebAssembly/binaryen/pull/1435#discussion_r169518704. This is one of the reasons why I suggest we have 80 cols limit, the same with most other C++ projects.

Conclusion

Currently we don't have any formatting rules and existing codebase is in many case formatted manually and inconsistently to create pretty alignments or more readable code. I think having any kind of formatting rule is better than no rule, and it also automates all the formatting tasks so we don't need to it manually anymore.

In #1407 we agreed on the line length of 100 cols, but here I also would like to endorse 80 columns because

• We can enable single line if and case statements without forcing too many existing two-line statements into single ones (Currently the .clang-format checked in does not allow single line if/case. We can add rules to allow them if we agree on this)
• Many people use multiple terminals vertically in a screen, and having the customary 80 cols is suitable for the workflow

But if people don't like that, I still think having any formatting rule, regardless of line length, is better than nothing. And if people think a mass-reformatting CL is not very desirable (I don't mind it though), I at least suggest we start applying it to new PRs.

[tlively]

🙌🙏🙌🙏🙌🙏🙌

I strongly support this proposal. 80-character lines would make me very happy. I would even go further and make the CI check that the formatting follows the rules. If the formatting of some section of code is important enough that it should be exempted from clang-format, then it is important enough to
explicitly mark as exempt.

For things like mult-line definitions, readability could be maintained by repeating the type on each line. It's a little more typing, but it's just as readable.

I also think a mass-reformatting CL is great, like ripping off a bandaid. It is only one extra level of git-blame to jump over and it is very clear that it needs to be jumped over. The immediate improvement in code quality is worth the pain IMO.

[kripken]

I support this. Personally I'd prefer things otherwise, but it sounds like it would make everyone else happier and more productive, and that's more important.

[kripken]

Just wanted to add that I think @aheejin's arguments here are very good, and are what convinced me to change my mind about this. Good point that any rule is better than none. We will need to be careful though about which files to exclude (because of things like that cashew::IString line quoted above), but hopefully we can have very few such exclusions.

I would personally prefer a higher number than 80 columns. If most people want 80 though, I'm ok with that. Rust has 100, which personally I like, but most projects (LLVM etc.) are 80, so that's fair enough.

[aheejin]

Thanks for the support! I opened #1986 to address concerns here.

[aheejin]

Closed by #2059.