[PATCH] D109557: Adds an AlignCloseBracket option

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D109557#2998727 <https://reviews.llvm.org/D109557#2998727>, @csmulhern wrote:

> In D109557#2998213 <https://reviews.llvm.org/D109557#2998213>, @HazardyKnusperkeks wrote:
>
>> With context he meant the diff context. https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
>
> Ah sorry about that. Done.

No Problem.

> In D109557#2998226 <https://reviews.llvm.org/D109557#2998226>, @HazardyKnusperkeks wrote:
>
>> You state in the documentation that it is also for angle brackets and more, but there are no test cases for that.
>
> Yeah, I wasn't sure exactly how to deal with this. The default behavior is already to align angle brackets and braces on newlines. See: https://github.com/llvm/llvm-project/blob/8a780a2f18c590e27e51a2ab3cc81b481c42b42a/clang/lib/Format/ContinuationIndenter.cpp#L341 (BreakBeforeClosingBrace is true when the block was started with a newline). Thus, you're already getting this behavior when CBAS_AlwaysBreak is set. I didn't want to make DontAlign (the default) explicitly opt out of this behavior. I guess we can narrow the scope of CloseBracketAlignmentStyle to just parenthesis, but that doesn't feel great either. What are your thoughts?

I haven't looked too much into it, my main point is that there should be tests for both variants of that option for braces, parenthesis, and angular braces, if they are handled by that option. Otherwise the documentation (and naming?) should be adapted. If the defaults differ, the option has to be reworked, for a finer control.

In D109557#2999085 <https://reviews.llvm.org/D109557#2999085>, @MyDeveloperDay wrote:

> This isn't really `AlignCloseBracket` but `BreakBeforeClosingParen` isn't it?

Yeah I thought that too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109557/new/

https://reviews.llvm.org/D109557