[tools] Allow argument/parameter bin packing in clang-format #21221

1. 
   jnewbery commented at 10:12 am on February 18, 2021: member

   clang-format documentation for BinPackArguments:

   If false, a function call’s arguments will either be all on the same line or will have one line each.

    0true:
    1void f() {
    2  f(aaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaa,
    3    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);
    4}
    5
    6false:
    7void f() {
    8  f(aaaaaaaaaaaaaaaaaaaa,
    9    aaaaaaaaaaaaaaaaaaaa,
   10    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);
   11}
   

   https://clang.llvm.org/docs/ClangFormatStyleOptions.html#configurable-format-style-options

   There’s no reason to forbid this format. Having multiple arguments or parameters per line can be just as readable as having one per line (and is certainly more readable than having extremely long lines).

2. [tools] Allow argument/parameter bin packing in clang-format 876ac3f6b6
3. 
   laanwj commented at 12:36 pm on February 18, 2021: member

   Good idea ACK 876ac3f6b62087fb5c22b0a477751895915d47b8
4. laanwj added the label Scripts and tools on Feb 18, 2021
5. 
   vasild commented at 6:02 pm on February 18, 2021: member

   Hmm, I think this is only relevant if there is a limit on the max line length, so that clang-format goes on to break/wrap the line (#21223 adds such a limit). Otherwise with or without this PR the following line remains unchanged:

   0    CNode* pnode = new CNode(id, nLocalServices, sock->Release(), addrConnect, CalculateKeyedNetGroup(addrConnect), nonce, addr_bind, pszDest ? pszDest : "", conn_type, /* inbound_onion */ false);
   

   If we put a limit on the length, then:

   true:

   0    CNode(NodeId id, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const CAddress& addrIn,
   1          uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn,
   2
   3
   4    CNode* pnode = new CNode(id, nLocalServices, sock->Release(), addrConnect,
   5                             CalculateKeyedNetGroup(addrConnect), nonce, addr_bind,
   6                             pszDest ? pszDest : "", conn_type,
   7                             /* inbound_onion */ false);
   

   false:

    0    CNode(NodeId id,
    1          ServiceFlags nLocalServicesIn,
    2          SOCKET hSocketIn,
    3          const CAddress& addrIn,
    4          uint64_t nKeyedNetGroupIn,
    5          uint64_t nLocalHostNonceIn,
    6          const CAddress& addrBindIn,
    7          const std::string& addrNameIn,
    8          ConnectionType conn_type_in,
    9          bool inbound_onion);
   10
   11
   12    CNode* pnode = new CNode(id,
   13                             nLocalServices,
   14                             sock->Release(),
   15                             addrConnect,
   16                             CalculateKeyedNetGroup(addrConnect),
   17                             nonce,
   18                             addr_bind,
   19                             pszDest ? pszDest : "",
   20                             conn_type,
   21                             /* inbound_onion */ false);
   

   Maybe it is subjective which one is easier to read. Personally I find the false variant (second) more readable. An example diff:

   true:

   0     CNode* pnode = new CNode(id, nLocalServices, sock->Release(), addrConnect,
   1-                             CalculateKeyedNetGroup(addrConnect), nonce, addr_bind,
   2+                             CalculateKeyedNetGroup(addrConnect), n0nce, addr_bind,
   3                              pszDest ? pszDest : "", conn_type,
   4                              /* inbound_onion */ false);
   

   false:

    0     CNode* pnode = new CNode(id,
    1                              nLocalServices,
    2                              sock->Release(),
    3                              addrConnect,
    4                              CalculateKeyedNetGroup(addrConnect),
    5-                             nonce,
    6+                             n0nce,
    7                              addr_bind,
    8                              pszDest ? pszDest : "",
    9                              conn_type,
   10                              /* inbound_onion */ false);
   

6. 
   jnewbery commented at 6:24 pm on February 18, 2021: member

   I think this is only relevant if there is a limit on the max line length, so that clang-format goes on to break/wrap the line (#21223 adds such a limit). Otherwise with or without this PR the following line remains unchanged…

   Without this change, clang-format will try to fix argument/parameter lists that are already bin packed into a single line.

7. 
   MarcoFalke commented at 6:45 pm on February 18, 2021: member

   review ACK 876ac3f6b62087fb5c22b0a477751895915d47b8

   Looks like this is making clang-format less restrictive. The new config allows single-line, one-per-line, and packed-lines.

8. vasild approved
9. 
   vasild commented at 9:05 am on February 19, 2021: member

   ACK 876ac3f6b62087fb5c22b0a477751895915d47b8

   I think this is only relevant if there is a limit on the max line length

   That’s not true, see the last row in the table below. @jnewbery, @MarcoFalke, you are right. To summarize:

   input	result after reformat, bin pack: false	result after reformat, bin pack: true
   long line	no change	no change
   manually formatted one per line	no change	no change
   manually formatted packed line	long line (joined)	no change

10. MarcoFalke merged this on Feb 19, 2021
11. MarcoFalke closed this on Feb 19, 2021

    ---

12. jnewbery deleted the branch on Feb 19, 2021
13. sidhujag referenced this in commit ae3a50e0c7 on Feb 19, 2021
14. DrahtBot locked this on Aug 16, 2022

Contributors
jnewbery laanwj vasild MarcoFalke

Labels
Scripts and tools