Marking as draft for now. I expect people will have opinions.

Concept ACK. And forbid refactoring pulls just to make lines shorter.

The primary issue I experience as a reviewer is reformatting of lines, especially function signatures or function calls, to make lines shorter.

@jonatack My take on this is that if I need to add an argument to a 230 char wide function declaration, then I'd much prefer to split that line then turn it into a 260 char wide function declaration. The alternative results in lines that just grow without bound.

... obfuscates the diff and makes review more fatiguing, difficult and error-prone than it otherwise needed to be.

In my experience, it's pretty easy to see which changes are whitespace only, eg in meld:

compared to this for adding a new argument to a very long line:

The example in the PR description is more readable to me in one line than split into several

Can you share a screenshot? In my editor, with a column width of 126 chars, it looks like:

which I find very difficult to read.

Concept ACK.
The long lines have been bothering me for quite some time and I think in the long-term following this guideline will increase code quality. Agree with hebasto though that refactor PRs with the sole goal of line-shortening should be discouraged. (On the other hand, I think many reasonable refactorings will as a side-effect lead to shorter lines.)

From a practical point of view, I personally (and know many people who do) enjoy being able to view two source code files side-by-side without line breaks on a single modern screen, and while 80 is likely too low, decades after 80x25 terminals were widespread (there has been an interesting discussion on the Linux Kernel Mailing List about this subject: https://lkml.org/lkml/2020/5/29/1038), a soft-limit of 100 seems to be a good compromise here. If 100 is still too low for some, the absolute maximum should be a number that is still visible on a single screen without line breaks. Lines longer than 180-200 chars are absolutely inacceptable, IMHO. And unfortunately, we have many instances of those, especially in src/validation.cpp.

The primary issue I experience as a reviewer is reformatting of lines, especially function signatures or function calls, to make lines shorter. It adds churn, obfuscates the diff and makes review more fatiguing, difficult and error-prone than it otherwise needed to be.

Tend to agree with jnewbery here, that this can be solved by right usage of the diff-tool. In an ideal world the creator of commit would always put review options suggestions in a commit message where the regular diff is hard to read. This is happening already, e.g. "best reviewed with -w --color-moved=..." etc. The drawback is that this doesn't work with the rendered diff online on github, but this shouldn't be encouraged anyways.

In the long term, after this rule will roughly be followed for quite some time, if there is still many instances where changes introduce line-breaks, there is more likely something wrong with the code. I don't think that having lots of functions/methods with 5+ parameters is desirable. Hence I would hope that in the long-term, with this rule applied the functions/methods are designed in a way that most calls would still likely fit in a single line.

@jonatack: Hm, I see your point of view. Though it's very reviewers-centric and not taking into account potential new contributors which could also be turned off just by seeing those long lines. (And overly huge function/method bodies, but that's another topic...)

I think in cases like #20788 where really lots of those line-breaks occur, the root of the problem is that stylistic changes and logical changes are mixed in one. It would make sense to first have an isolated commits that solely refactors the line-breaks, which could in theory be verified automatically (not sure if such a tool already exists, but I think it could be possible to "normalize" all white-space in two source files and then compare if they are still identical) and then based on this do the actual function signature changes. Reviewing the logical change would then be a piece of cake.

For example, instead of having a single commit that suffers from your described problem:

-static bool Socks5(const std::string& strDest, int port, const ProxyCredentials *auth, const SOCKET& hSocket)
+static bool Socks5(const std::string& strDest,
+                   int port,
+                   const ProxyCredentials* auth,
+                   const Sock& sock)

you would have a pure-line-break-refactor (potentially machine-verified) commit (1/2)

-static bool Socks5(const std::string& strDest, int port, const ProxyCredentials *auth, const SOCKET& hSocket)
+static bool Socks5(const std::string& strDest,
+                   int port,
+                   const ProxyCredentials *auth,
+                   const SOCKET& hSocket)

and then a logical human-verified commit (2/2) that focusses on the actual signature changes

 static bool Socks5(const std::string& strDest,
                    int port,
-                   const ProxyCredentials *auth,
-                   const SOCKET& hSocket)
+                   const ProxyCredentials* auth,
+                   const Sock& sock)

That's just an idea, don't know if it's worth chasing it further. My point is that it's not an absolute unavoidable consequence of proposed line-length limits that there has to be churn. Like already described in a previous comment, I'd assume that the changes get less and less painful for reviewers once the first line-shortening is done. For newly written code, exceeding the line limit too much will serve as kind of a warning sign that the code structure is likely flawed and should be reconsidered.

You can enforce line lengths while still allowing exceptions to the rule via the following:

// clang-format off
this is a block where ...
... very long lines ...
... and other formatting exceptions ....
... are allowed ....
... but there always need to be ...
... a very good reason for that
// clang-format on

We use this for Trezor and it's working perfectly. Code that deserves special handling is explicitly marked and the rule is followed everywhere else.

We also decided to follow Google practices (BasedOnStyle: Google which defines line length = 80 and tab width = 2 among other things) for the following reasons:

• while most of the editors can show 100+ characters on modern displays without any problems; they can't show 160+ characters required for the side-by-side diffs - not having to scroll makes code review much less painful
• using tab width of 4 or more is very limiting when using only 80 chars per line; tab width of 2 saves a lot of precious line space and still provides good readability

To wrap it up - I think it makes sense to have either line length of 100 + tab width of 4 or line length of 80 + tab width of 2, while the latter makes side-by-side reviews much more pleasant experience.

I just shared this to explain what we went through; I am not trying to expose our/Google formatting rules on the bitcoin codebase and I'll be happy with any outcome of this discussion.

Concept ACK. It is cleaner and easier to compare in diffs.

Concept ACK

concept ACK, code is much easier to read when I can see it :)

I'd personally prefer 120 characters, esp with some of our long function names, but its nbd and would rather aim for 100 than off-the-page long.

Concept ACK: when it comes to degrees of freedom in code formatting less is more :)

Personally I'm a big fan of black, clang-format, gofmt, etc: computers have a comparative advantage when it comes to non-creative tasks :)

Concept ACK, I think this notes what we currently try to do.

Concept ACK. And forbid refactoring pulls just to make lines shorter.

@hebasto that's already noted:

Do not submit patches solely to modify the style of existing code.

I'm fine with this, as long as it's meant as a review guideline for new code and not too zealously enforced. I wouldn't like to see e.g. PRs that just change line length all over the codebase.

Concept ACK, I believe documenting general preferences (which we already have) would reduce the amount of time spent on style when coding and reviewing. I think it'd be most effective to just point to a tool or script that we can use to do it automatically.

ACK aa929ab - this is basically just something to point too when a PR has unreasonably long lines for no particularly reason.

I'm fine with this, as long as it's meant as a review guideline for new code and not too zealously enforced. I wouldn't like to see e.g. PRs that just change line length all over the codebase.

Agree. PRs opened just to shuffle code around and reduce line length will be closed.

Otherwise. I'm not sure what's left to do here?

ACK aa929ab

This seems to have broad support, so I'm moving it out of draft.

Several reviewers have highlighted that there shouldn't be PRs just to 'fix up' long lines. That's already covered further up in the developer notes:

Do not submit patches solely to modify the style of existing code.

ACK aa929ab