docs: update developer notes to discourage very long lines

jnewbery commented at 10:04 am on January 22, 2021: member

Mandatory rules on line lengths are bad - there will always be cases where a longer line is more readable than the alternative.

However, very long lines for no good reason do hurt readability. For example, this declaration in validation.h is 274 chars:

0    bool ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool.cs);

That won’t fit on one line without wrapping on my 27" monitor with a comfortable font size. Much easier to read is something like:

0    bool ConnectTip(BlockValidationState& state, const CChainParams& chainparams,
1                    CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock,
2                    ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool)
3        EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool.cs);

Therefore, discourage (don’t forbid) line lengths greater than 100 characters in our developer style guide.

100 chars is somewhat arbitrary. The old standard was 80, but that seems very limiting with modern displays.

[docs] Update developer notes to discourage very long lines aa929abf8d

jnewbery commented at 10:04 am on January 22, 2021: member

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

fanquake added the label Docs on Jan 22, 2021

hebasto commented at 10:48 am on January 22, 2021: member

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

jnewbery commented at 12:10 pm on January 22, 2021: member

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.

theStack commented at 1:51 pm on January 22, 2021: member

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.

theStack commented at 3:22 pm on January 22, 2021: member

@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:

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

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

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

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

0 static bool Socks5(const std::string& strDest,
1                    int port,
2-                   const ProxyCredentials *auth,
3-                   const SOCKET& hSocket)
4+                   const ProxyCredentials* auth,
5+                   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.

prusnak commented at 1:37 pm on January 23, 2021: contributor

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

0// clang-format off
1this is a block where ...
2... very long lines ...
3... and other formatting exceptions ....
4... are allowed ....
5... but there always need to be ...
6... a very good reason for that
7// 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.

lontivero commented at 4:03 pm on January 25, 2021: contributor

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

michaelfolkson commented at 5:30 pm on January 25, 2021: contributor

Concept ACK

amitiuttarwar commented at 9:05 pm on January 25, 2021: contributor

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.

practicalswift commented at 10:58 am on January 26, 2021: contributor

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 :)

promag commented at 12:03 pm on January 27, 2021: member

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.

laanwj commented at 4:43 pm on January 27, 2021: member

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.

luke-jr changes_requested

luke-jr commented at 9:09 pm on January 28, 2021: member

Concept NACK.

Editors can automatically wrap long lines when displaying, and to the editor’s preferred style and length. Manually wrapped lines just make it less nice to work at other wrapping lengths.

glozow commented at 3:12 pm on January 29, 2021: member

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.

fanquake commented at 3:37 am on February 13, 2021: member

ACK aa929abf8dc022e900755234c857541faeea8239 - 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?

practicalswift commented at 7:03 am on February 13, 2021: contributor

ACK aa929abf8dc022e900755234c857541faeea8239

jnewbery marked this as ready for review on Feb 13, 2021

jnewbery commented at 5:47 pm on February 13, 2021: member

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:

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

theStack approved

theStack commented at 6:29 pm on February 13, 2021: member

ACK aa929abf8dc022e900755234c857541faeea8239

amitiuttarwar commented at 2:30 am on February 14, 2021: contributor

ACK aa929abf8dc022e900755234c857541faeea8239

glozow commented at 4:53 am on February 14, 2021: member

ACK https://github.com/bitcoin/bitcoin/commit/aa929abf8dc022e900755234c857541faeea8239

jnewbery renamed this:
~~[RFC] docs: update developer notes to discourage very long lines~~
docs: update developer notes to discourage very long lines
on Feb 14, 2021

MarcoFalke merged this on Feb 14, 2021

MarcoFalke closed this on Feb 14, 2021

jnewbery deleted the branch on Feb 14, 2021

sidhujag referenced this in commit 6e34d992b7 on Feb 14, 2021

PastaPastaPasta referenced this in commit 66e4dee269 on Jun 27, 2021

PastaPastaPasta referenced this in commit a1e688f607 on Jun 28, 2021

PastaPastaPasta referenced this in commit 2a40dbc9f0 on Jun 29, 2021

PastaPastaPasta referenced this in commit 1ba93cdb75 on Jul 1, 2021

PastaPastaPasta referenced this in commit da583688ab on Jul 1, 2021

PastaPastaPasta referenced this in commit 008b8de0c7 on Jul 15, 2021

PastaPastaPasta referenced this in commit 2b19a8a14f on Jul 15, 2021

PastaPastaPasta referenced this in commit 5a7e219d81 on Jul 16, 2021

gabriel-bjg referenced this in commit 2442258987 on Jul 16, 2021

DrahtBot locked this on Aug 16, 2022

docs: update developer notes to discourage very long lines #20986