Change coding style for argument alignment?

Hi,

LLVM currently uses a coding style where arguments and parameters need to be aligned to the opening parenthesis “(”.

someFunctionCall(Arg1, Arg2,

Arg3, Arg4);

This style guideline is unfortunate for a couple of reasons:

  1. If a function is renamed, it is necessary to also reindent the arguments at all call-sites. For functions with many or complex arguments, this may require significant reflow.

  2. It causes significant right-ward drift. Especially for declarations, it’s somewhat common for code ending up like this…

Foo SomeClassName::someMethodName(Bar &Arg1,
Bar &Arg2,
Bar &Arg3,
Bar &Arg4) {

… because there is just enough space to fit each argument individually, but still a need to break each one out on a separate line. Closure arguments are another common case of very awkward formatting.

  1. There are some arcane rules about when this is not the preferred style and you’re supposed to indent arguments on the following line instead.

Is there any chance that the style could be changed towards indenting (but not aligning) the following line instead?

someFunctionCall(
Arg1, Arg2, Arg3, Arg4);

This is unaffected by renames, does not cause rightward drift and results in very predictable formatting.

Based on past discussions, LLVM seems to be open to improving coding style for the better, so I thought I’d give this a shot, as this is a continuous source of friction for me.

Regards,

Nikita

I support this proposal. I very much dislike the current function parameter formatting (in function declarations/definitions). It just adds a bunch of whitespace without improvement in readability.

In practice there are already various mixtures of formatting being used. For example:

bool tailDuplicate(bool IsSimple,

MachineBasicBlock *TailBB,

MachineBasicBlock *ForcedLayoutPred,

SmallVectorImpl<MachineBasicBlock *> &TDBBs,

SmallVectorImpl<MachineInstr *> &Copies,

SmallVectorImpl<MachineBasicBlock *> *CandidatePtr);

void appendCopies(MachineBasicBlock *MBB,

SmallVectorImpl<std::pair<Register, RegSubRegPair>> &CopyInfos,

SmallVectorImpl<MachineInstr *> &Copies);

void removeDeadBlock(

MachineBasicBlock *MBB,

function_ref<void(MachineBasicBlock *)> *RemovalCallback = nullptr);

These are 3 adjacent declarations in the same file (llvm/include/llvm/CodeGen/TailDuplicator.h).

I don’t think that the alternative format would make call sites immune to renaming, since it can still cause fewer or more function arguments to fit in a given line of code. I still think it’s a worthwhile change in style.

(probably worth using a monospaced font when discussing indent styles in HTML email, otherwise it’s a bit hard to tell what’s being discussed)

I support this proposal. I very much dislike the current function parameter formatting (in function declarations/definitions). It just adds a bunch of whitespace without improvement in readability.

In practice there are already various mixtures of formatting being used. For example:

bool tailDuplicate(bool IsSimple,

MachineBasicBlock *TailBB,

MachineBasicBlock *ForcedLayoutPred,

SmallVectorImpl<MachineBasicBlock *> &TDBBs,

SmallVectorImpl<MachineInstr *> &Copies,

SmallVectorImpl<MachineBasicBlock *> *CandidatePtr);

void appendCopies(MachineBasicBlock *MBB,

SmallVectorImpl<std::pair<Register, RegSubRegPair>> &CopyInfos,

SmallVectorImpl<MachineInstr *> &Copies);

Looks like “appendCopies” is the odd one out here. Probably someone didn’t reindent when the name was changed. Hmm, nope, it was checked in that way - but it doesn’t match any style I think we’ve ever had, so it’s not really an argument for/against any particular format - the author seems to have uesd right alignment to fit the long name. (possible that our style guide didn’t or doesn’t specify what to do when things are too long - but I think generally, the answer is what’s done for removeDeadBlock: Fallback to a double indent from the start, and this is what clang-format does with its LLVM style interpretation)

void removeDeadBlock(

MachineBasicBlock *MBB,

function_ref<void(MachineBasicBlock *)> *RemovalCallback = nullptr);

These are 3 adjacent declarations in the same file (llvm/include/llvm/CodeGen/TailDuplicator.h).

I don’t think that the alternative format would make call sites immune to renaming, since it can still cause fewer or more function arguments to fit in a given line of code. I still think it’s a worthwhile change in style.

I don’t have strong feelings either way - mostly of the opinion that it’s a “clang-format and forget about it” sort of thing. (which isn’t to say formatting issues can’t rise to the level of “this is actively harmful to readability of the code” but this one doesn’t rise to that level from my perspective at least)

  • Dave

Hi,

LLVM currently uses a coding style where arguments and parameters need to be aligned to the opening parenthesis “(”.

someFunctionCall(Arg1, Arg2,

Arg3, Arg4);

This style guideline is unfortunate for a couple of reasons:

  1. If a function is renamed, it is necessary to also reindent the arguments at all call-sites. For functions with many or complex arguments, this may require significant reflow.

Why is this a problem though? This is all automated by clang-format in practice.

  1. It causes significant right-ward drift. Especially for declarations, it’s somewhat common for code ending up like this…

Foo SomeClassName::someMethodName(Bar &Arg1,
Bar &Arg2,
Bar &Arg3,
Bar &Arg4) {

… because there is just enough space to fit each argument individually, but still a need to break each one out on a separate line. Closure arguments are another common case of very awkward formatting.

Seems like clang-format has some heuristics for this, I wrote this in a file:

Foo SomeClassName::someveryveryveryveryverylongMethodName(Bar &Arg1, Bar &Arg2, Bar &Arg3, Bar &Arg4, Bar &Arg5, Bar &Arg6, Bar &Arg7, Bar &Arg8) {}
Foo SomeClassName::someveryveryveryveryveryverylongMethodName(Bar &Arg1, Bar &Arg2, Bar &Arg3, Bar &Arg4, Bar &Arg5, Bar &Arg6, Bar &Arg7, Bar &Arg8) {}
Foo SomeClassName::someMethodName(Bar &Arg1, Bar &Arg2, Bar &Arg3, Bar &Arg4, Bar &Arg5, Bar &Arg6, Bar &Arg7, Bar &Arg8) {}

and clang-format print it this way:

Foo SomeClassName::someveryveryveryveryverylongMethodName(Bar &Arg1, Bar &Arg2,
Bar &Arg3, Bar &Arg4,
Bar &Arg5, Bar &Arg6,
Bar &Arg7,
Bar &Arg8) {}
Foo SomeClassName::someveryveryveryveryveryverylongMethodName(
Bar &Arg1, Bar &Arg2, Bar &Arg3, Bar &Arg4, Bar &Arg5, Bar &Arg6, Bar &Arg7,
Bar &Arg8) {}
Foo SomeClassName::someMethodName(Bar &Arg1, Bar &Arg2, Bar &Arg3, Bar &Arg4,
Bar &Arg5, Bar &Arg6, Bar &Arg7, Bar &Arg8) {}

  1. There are some arcane rules about when this is not the preferred style and you’re supposed to indent arguments on the following line instead.

Is there any chance that the style could be changed towards indenting (but not aligning) the following line instead?

someFunctionCall(
Arg1, Arg2, Arg3, Arg4);

Is there an existing clang-format setting to hook up to already?

(probably worth using a monospaced font when discussing indent styles in HTML email, otherwise it’s a bit hard to tell what’s being discussed)

Hm. It was Consolas at 10pt. What were you seeing?

(probably worth using a monospaced font when discussing indent styles in HTML email, otherwise it’s a bit hard to tell what’s being discussed)

Hm. It was Consolas at 10pt. What were you seeing?

Ah, indeed. Seems Consolas isn’t commonly available on Mac ( https://www.cssfontstack.com/Consolas ) so it renders with some other (variable width) font, unfortunately. I do see it formatted correctly when I use a Windows computer.

Nikita Popov via llvm-dev <llvm-dev@lists.llvm.org> writes:

2. It causes significant right-ward drift. Especially for declarations,
it's somewhat common for code ending up like this...

   Foo SomeClassName::someMethodName(Bar &Arg1,
                                      Bar &Arg2,
                                      Bar &Arg3,
                                      Bar &Arg4) {

... because there is just enough space to fit each argument individually,
but still a need to break each one out on a separate line. Closure
arguments are another common case of very awkward formatting.

FWIW, I have actually drifted toward this style in my own non-LLVM code.
That is, if there are more arguments than can fit on a single line, each
gets its own line. For example:

void short_args(double an_argument_name, int another_parameter);

void long_args(double first_argument,
               int second_argument,
               int *third_argument,
               int fourth argument);

I find this easier to read than:

void long_args(double first_argument, int second_argument,
               int *third_argument, int fourth argument);

It's easier for me because I need only scan vertically to find the
arguments, not both vertically and horizontally.

Long function names can be problematic and you're right about changing
names requiring reformatting. I've toyed with this style:

void a_really_long_and_descriptive_function_name(
       double first_argument,
       int second_argument,
       int *third_argument,
       int fourth argument);

I haven't yet convinced myself that this is a good idea. It looks too
jumbled to me and is more difficult to immediately find the start of the
argument list.

                 -David

Nikita Popov via llvm-dev <llvm-dev@lists.llvm.org> writes:

  1. It causes significant right-ward drift. Especially for declarations,
    it’s somewhat common for code ending up like this…

Foo SomeClassName::someMethodName(Bar &Arg1,
Bar &Arg2,
Bar &Arg3,
Bar &Arg4) {

… because there is just enough space to fit each argument individually,
but still a need to break each one out on a separate line. Closure
arguments are another common case of very awkward formatting.

FWIW, I have actually drifted toward this style in my own non-LLVM code.
That is, if there are more arguments than can fit on a single line, each
gets its own line. For example:

void short_args(double an_argument_name, int another_parameter);

void long_args(double first_argument,
int second_argument,
int *third_argument,
int fourth argument);

This also saves the annoying pain of everything shifting within a line just because an argument got added (which messes up line-based diffs). That pain is most noticeable when adding new members (or just updating initializers) in member initializer lists for constructors.

I support this change. I think you’ve done a good job of laying out a principled reason that this approach is better, and I don’t see any significant advantages to the current approach. We should obviously keep the arguments on the same line if they fit though.

-Chris

There's a big argument against the current standard, which is that if you have several functions next to each other, they are all formatted differently. Very confusing on the eyes.

My personal preference would be either (a) all arguments on one line if there's room (either with the function name, or the line below), or (b) one argument per line with an indent of either one or two from the start of the function name.

Cheers,
Wol

Hi,
LLVM currently uses a coding style where arguments and parameters need to be aligned to the opening parenthesis “(”.

someFunctionCall(Arg1, Arg2,
Arg3, Arg4);

This style guideline is unfortunate for a couple of reasons:

  1. If a function is renamed, it is necessary to also reindent the arguments at all call-sites. For functions with many or complex arguments, this may require significant reflow.

  2. It causes significant right-ward drift. Especially for declarations, it’s somewhat common for code ending up like this…

Foo SomeClassName::someMethodName(Bar &Arg1,
Bar &Arg2,
Bar &Arg3,
Bar &Arg4) {

… because there is just enough space to fit each argument individually, but still a need to break each one out on a separate line. Closure arguments are another common case of very awkward formatting.

  1. There are some arcane rules about when this is not the preferred style and you’re supposed to indent arguments on the following line instead.

Is there any chance that the style could be changed towards indenting (but not aligning) the following line instead?

someFunctionCall(
Arg1, Arg2, Arg3, Arg4);

This is unaffected by renames, does not cause rightward drift and results in very predictable formatting.

Based on past discussions, LLVM seems to be open to improving coding style for the better, so I thought I’d give this a shot, as this is a continuous source of friction for me.

I support this change. I think you’ve done a good job of laying out a principled reason that this approach is better, and I don’t see any significant advantages to the current approach. We should obviously keep the arguments on the same line if they fit though.

There’s a big argument against the current standard, which is that if
you have several functions next to each other, they are all formatted
differently. Very confusing on the eyes.

My personal preference would be either (a) all arguments on one line if
there’s room (either with the function name, or the line below), or (b)
one argument per line with an indent of either one or two from the start
of the function name.

To add my €0.02, whatever the common form is there are times when it makes more sense to escape from it. A common example is where a function has arguments that come in pairs – common in UI toolkits with x/y or w/h pairs – and the formatting should reflect that. It’s less common in LLVM, though I’ve seen it happen. Being slightly more relaxed with such things would be great.

That said, the proposed change is clearly better even in this case, because while

someFunctionCall(
x1, y1,
x2, y2);

is best, I’d say that

someFunctionCall(
x1,
y1,
x2,
y2);

is far better than the

someFunctionCall(
x1, y2, x2,
y2);

that the current style likes to generate.

Something analogous happens with &&- or ||- chains in conditionals.

Cheers,
Nicolai

Given that this is all being auto-formatted, should we implement that by having the rule that the arguments will be auto-indented, but won't be re-laid out if there's a comment marker at the end, eg

someFunctionCall(
      x1, y1, //
      x2, y2); //

Whether there is a comment there or not, the marker will be preserved and suppress the re-layout.

Cheers,
Wol