[clang-format] AlignAfterOpenBracket tries to do too much and doesn't do what it says

[davidstone]

AlignAfterOpenBracket has four configuration values:

• Align
• DontAlign
• AlwaysBreak
• BlockIndent

This configuration option is trying to accomplish the following separate goals, assuming you need to insert a line break between arguments:

1. Do you use one indentation level or do you align to the opening bracket? (Align vs DontAlign)
2. Do you start the first argument on the same line as the open bracket or the next line? (AlwaysBreak vs others)
3. Do you put the closing bracket on the same line as the last argument or a new line? (BlockIndent vs others)

My style (and one I've seen other people use) is to always put a new argument on each line, with one level of indentation, and have the closing bracket on its own line (I think I've seen this indented or not? I prefer not), by analogy to curly braces {}:

function(
    a,
    b,
    c
);

The behavior of BlockIndent also does not match the documentation. The documentation states:

BAS_BlockIndent (in configuration: BlockIndent) Always break after an open bracket, if the parameters don’t fit on a single line. Closing brackets will be placed on a new line.

However, given

	using type = std::remove_cv_t<
		add_common_cv_reference<
			std::common_type_t<std::decay_t<T0>, std::decay_t<T1>>,
			T0,
			T1
		>
	>;

and AlignAfterOpenBracket set to BlockIndent, clang-format turns that into

	using type = std::remove_cv_t<
		add_common_cv_reference<
			std::common_type_t<std::decay_t<T0>, std::decay_t<T1>>,
			T0,
			T1>>;

It's also unclear whether BlockIndent is supposed to do alignment or indentation, but in my code it looks like it inconsistently does both? AlwaysBreak is similarly ambiguous in its documentation.

To summarize, my preferred resolution of this issue is that we would have something like:

AlignWithOpenBracket

true is behavior of current Align, false is behavior of current DontAlign: this option controls only whether new arguments are aligned to be right after the open bracket if they don't fit on the first line.

// true
function(a,
         b,
         c);

vs

// false
function(a,
    b,
    c);

BreakAfterOpenBracket

true means that if arguments don't all fit on one line, the first argument starts on a new line, false means the first one goes on the same line as the open bracket.

// true
function(
         a,
         b,
         c);

vs

// false
function(a,
         b,
         c);

BreakBeforeCloseBracket

true means that if arguments don't all fit on one line, the closing bracket starts a new line, false means it goes on the same line as the final argument.

// true
function(a,
         b,
         c
);

vs

// false
function(a,
         b,
         c);

So for me to get my preferred formatting, I would have

AlignWithOpenBracket: false
BreakAfterOpenBracket: true
BreakBeforeCloseBracket: true

It is not clear to me what any of the four current options correspond to exactly in this system, based on documentation and current behavior.

[gedare]

So for me to get my preferred formatting, I would have

AlignWithOpenBracket: false
BreakAfterOpenBracket: true
BreakBeforeCloseBracket: true

It is not clear to me what any of the four current options correspond to exactly in this system, based on documentation and current behavior.

This is BlockIndent without bin packing, but you are expecting it to do too much based on the documentation, because it "only applies to braced initializer lists (when Cpp11BracedListStyle is true) and parentheses."

Note that your first example is supported:

echo "function(
    a,
    b,
    c
);" | clang-format --style="{BasedOnStyle: llvm, AlignAfterOpenBracket: BlockIndent, BinPackParameters: false, ColumnLimit: 10}"
function(
    a,
    b,
    c
);

What you ask is a major refactoring of more or less working code. Addition of support for templates and lambdas in BlockIndent is likely an easier request to make.

[leijurv]

This case

	using type = std::remove_cv_t<
		add_common_cv_reference<
			std::common_type_t<std::decay_t<T0>, std::decay_t<T1>>,
			T0,
			T1
		>
	>;

is now possible as of commit d2b45ce

It is tested:

  // Test from https://github.com/llvm/llvm-project/issues/80049:
  verifyFormat(
      "using type = std::remove_cv_t<\n"
      "    add_common_cv_reference<\n"
      "        std::common_type_t<std::decay_t<T0>, std::decay_t<T1>>,\n"
      "        T0,\n"
      "        T1\n"
      "    >\n"
      ">;",
      "using type = std::remove_cv_t<\n"
      "    add_common_cv_reference<\n"
      "        std::common_type_t<std::decay_t<T0>, std::decay_t<T1>>,\n"
      "        T0,\n"
      "        T1>>;",
      Style);

d2b45ce#diff-3f6f57cda9809a57c5b79e22b4181b3f3aaac7216262d0ef44108f39b0443e9bR11276-R11290

[zhengchenliang]

    if (pthread_mutex_init(&pth1stat, NULL) != 0
      || pthread_mutex_init(&thd1list, NULL) != 0
      || pthread_mutex_init(&tsk1list, NULL) != 0
      || pthread_cond_init(&tsk2lnch, NULL) != 0
      || pthread_mutex_init(&tsk1full, NULL) != 0
      || pthread_cond_init(&tsk2recv, NULL) != 0
    )
    {

Is this supported? No matter what I do it will automatically put the ) to

    if (pthread_mutex_init(&pth1stat, NULL) != 0
      || pthread_mutex_init(&thd1list, NULL) != 0
      || pthread_mutex_init(&tsk1list, NULL) != 0
      || pthread_cond_init(&tsk2lnch, NULL) != 0
      || pthread_mutex_init(&tsk1full, NULL) != 0
      || pthread_cond_init(&tsk2recv, NULL) != 0)
    {

[MikeWeller]

I'm also running into the need for more fine-grained control over these behaviours. In my case I want AlwaysBreak but with the continuation indent intead of aligning with the open paren/brace.

edit: It seems there may be some bug I'm running into, because using AlwaysBreak sometimes aligns the parameters by one continuation indent, but sometimes still tries to align with the open paren.

i.e. in some code I get:

     xxxx::xxxxxxxxxxxxxxxxxxxxxxxxxxx::xxxxxxxxxxxxxxx(
                                                        xx,
                                                        xxxxx.xxxxxxxxxxxx(),
                                                        xxxxxxxx,
                                                        xx);

in other code I get (what I want):

    x::xxxxxxxxxxxxxxxxxxxxxx(
        xxxxxx,
        "xxxxxxxxxxxxxxxxxxxx",
        xxxxxxxxxxxxxx,
        xxxxx);

[gedare]

I'm also running into the need for more fine-grained control over these behaviours. In my case I want AlwaysBreak but with the continuation indent intead of aligning with the open paren/brace.

edit: It seems there may be some bug I'm running into, because using AlwaysBreak sometimes aligns the parameters by one continuation indent, but sometimes still tries to align with the open paren.

i.e. in some code I get:

     xxxx::xxxxxxxxxxxxxxxxxxxxxxxxxxx::xxxxxxxxxxxxxxx(
                                                        xx,
                                                        xxxxx.xxxxxxxxxxxx(),
                                                        xxxxxxxx,
                                                        xx);

in other code I get (what I want):

    x::xxxxxxxxxxxxxxxxxxxxxx(
        xxxxxx,
        "xxxxxxxxxxxxxxxxxxxx",
        xxxxxxxxxxxxxx,
        xxxxx);

This appears to be a different issue. Please provide a concise example with style that can reproduce the unwanted behavior.

[gedare]

    if (pthread_mutex_init(&pth1stat, NULL) != 0
      || pthread_mutex_init(&thd1list, NULL) != 0
      || pthread_mutex_init(&tsk1list, NULL) != 0
      || pthread_cond_init(&tsk2lnch, NULL) != 0
      || pthread_mutex_init(&tsk1full, NULL) != 0
      || pthread_cond_init(&tsk2recv, NULL) != 0
    )
    {

Is this supported? No matter what I do it will automatically put the ) to

    if (pthread_mutex_init(&pth1stat, NULL) != 0
      || pthread_mutex_init(&thd1list, NULL) != 0
      || pthread_mutex_init(&tsk1list, NULL) != 0
      || pthread_cond_init(&tsk2lnch, NULL) != 0
      || pthread_mutex_init(&tsk1full, NULL) != 0
      || pthread_cond_init(&tsk2recv, NULL) != 0)
    {

It is not supported. With the BlockIndent option breaking before the closing bracket only happens if there is a break after the opening bracket.

[har-keba]

Need this fixed too:

Instead of

// Line break because of max line length limit
CMessageSystem::Error(
   COMPONENT_NUMBER_MESSAGE_SYS, mCfgHdl, eMsgSys_LibCallFailed, "%s%s%s%d", mErrorName, scMsgSysLibName, "MsgSysStartIo", rtRet
);

I want

// Line break because of max line length limit
CMessageSystem::Error(
   COMPONENT_NUMBER_MESSAGE_SYS, mCfgHdl, eMsgSys_LibCallFailed, "%s%s%s%d", mErrorName, scMsgSysLibName, "MsgSysStartIo", rtRet);

BreakBeforeCloseBracket would fix this!
Would make code more readable to distinguish better between function definitions and function calls!