clang-format incorrectly indents wrapped closing parenthesis

[owenpan]

Summary

See https://bugs.llvm.org/show_bug.cgi?id=41413

[MyDeveloperDay]

clang/lib/Format/ContinuationIndenter.cpp

968 ↗

(On Diff #194052)

Do you think the colon you have here in your example is really a TT_CtorInitializerColon, I'm not sure if you need to be specific?

I wonder how this might interfere with the Style.BreakConstructorInitializers, I'm a little unclear of the before and after you are trying to fix, could you add something to the description to help me understand?

[klimek]

The previous behavior looks intentional, and much more regular. I'd be curious why you think the proposed behavior is more readable.

[MyDeveloperDay]

maybe add the following as a test because I think it shows the inconsistency

void Foo::bar( // some comment
) {}

void Foo::bar( // some comment
) const {}

[owenpan]

In D60374#1457638, @klimek wrote:

The previous behavior looks intentional, and much more regular. I'd be curious why you think the proposed behavior is more readable.

I see the bug in the inconsistency for the code below:

int Foo::getter(
    //
) const {
  return foo;
}

void Foo::setter(
    //
) {
  foo = 1;
}

The closing parenthesis is indented in getter but not setter.

[owenpan]

In D60374#1457639, @MyDeveloperDay wrote:

maybe add the following as a test because I think it shows the inconsistency

void Foo::bar( // some comment
) {}

void Foo::bar( // some comment
) const {}

I had the setter code in the new test case to show the inconsistency but then took it out; I don't want to add a (possible) duplicate even though I didn't find the test case for Lines 953-957 of ContinuationIndenter.cpp. I will put my setter test case back in.

[MyDeveloperDay]

LGTM , if you also think the test will help show the use case then please add it, otherwise this revision notes might be information enough

[owenpan]

clang/lib/Format/ContinuationIndenter.cpp

968 ↗

(On Diff #194052)

Good point with using TT_CtorInitializerColon instead!

Although I had tested it with different combinations of Style.BreakConstructorInitializers settings and didn't find any interference, it's probably not an inconsistency in the case of the colon based on your and @klimek's comments.

[owenpan]

In D60374#1457693, @MyDeveloperDay wrote:

LGTM , if you also think the test will help show the use case then please add it, otherwise this revision notes might be information enough

Thanks! I just got the alternative patch ready. Should I discard it and just commit this one?

[MyDeveloperDay]

In D60374#1457705, @owenpan wrote:

In D60374#1457693, @MyDeveloperDay wrote:

LGTM , if you also think the test will help show the use case then please add it, otherwise this revision notes might be information enough

Thanks! I just got the alternative patch ready. Should I discard it and just commit this one?

Maybe just update this revision with the new diff.

[owenpan]

Updated the diff.

cfe/trunk/lib/Format/ContinuationIndenter.cpp

if ((Current.isOneOf(tok::r_brace, tok::r_square) ||
       State.Stack.size() > 1) {
     if (Current.closesBlockOrBlockTypeList(Style))
       return State.Stack[State.Stack.size() - 2].NestedBlockIndent;
     if (Current.MatchingParen &&
         Current.MatchingParen->BlockKind == BK_BracedInit)
       return State.Stack[State.Stack.size() - 2].LastSpace;
     return State.FirstIndent;
   }
-  // Indent a closing parenthesis at the previous level if followed by a semi or
-  // opening brace. This allows indentations such as:
+  // Indent a closing parenthesis at the previous level if followed by a semi,
+  // const, or opening brace. This allows indentations such as:
   //     foo(
   //       a,
   //     );
+  //     int Foo::getter(
+  //         //
+  //     ) const {
+  //       return foo;
+  //     }
   //     function foo(
   //       a,
   //     ) {
   //       code(); //
   //     }
   if (Current.is(tok::r_paren) && State.Stack.size() > 1 &&
-      (!Current.Next || Current.Next->isOneOf(tok::semi, tok::l_brace)))
+      (!Current.Next ||
+       Current.Next->isOneOf(tok::semi, tok::kw_const, tok::l_brace)))
     return State.Stack[State.Stack.size() - 2].LastSpace;
   if (NextNonComment->is(TT_TemplateString) && NextNonComment->closesScope())
     return State.Stack[State.Stack.size() - 2].LastSpace;
   if (Current.is(tok::identifier) && Current.Next &&
       (Current.Next->is(TT_DictLiteral) ||
        ((Style.Language == FormatStyle::LK_Proto ||
          Style.Language == FormatStyle::LK_TextProto) &&
         Current.Next->isOneOf(tok::less, tok::l_brace))))

cfe/trunk/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, ConfigurableContinuationIndentWidth) {
   SixIndent.ContinuationIndentWidth = 6;
 
   EXPECT_EQ("int i =\n"
             "      longFunction(\n"
             "            arg);",
             format("int i = longFunction(arg);", SixIndent));
 }
 
+TEST_F(FormatTest, WrappedClosingParenthesisIndent) {
+  FormatStyle Style = getLLVMStyle();
+  verifyFormat(
+      "int Foo::getter(\n"
+      "    //\n"
+      ") const {\n"
+      "  return foo;\n"
+      "}",
+      Style);
+  verifyFormat(
+      "void Foo::setter(\n"
+      "    //\n"
+      ") {\n"
+      "  foo = 1;\n"
+      "}",
+      Style);
+}
+
 TEST_F(FormatTest, SpacesInAngles) {
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpacesInAngles = true;
 
   verifyFormat("static_cast< int >(arg);", Spaces);
   verifyFormat("template < typename T0, typename T1 > void f() {}", Spaces);
   verifyFormat("f< int, float >();", Spaces);
   verifyFormat("template <> g() {}", Spaces);