[clang-format] [PR41170] Break after return type ignored with certain comments positions

[MyDeveloperDay]

Summary

Addresses https://bugs.llvm.org/show_bug.cgi?id=41170

The AlwaysBreakAfterReturn type setting can go wrong if the line ends with a comment

void foo() /* comment */

or

void foo() // comment

It will incorrectly see such functions as Declarations and not Definitions

The following code addresses this by looking for function which end with ; <comment> rather than just ; or <comment>

[owenpan]

clang/lib/Format/TokenAnnotator.h

114–116 ↗

(On Diff #194021)

May I suggest the following instead?

return !endsWith(tok::semi);

[owenpan]

I added @sammccall to the reviewer list as I'm not sure if we need all these test cases or if there are overlaps among them. It seems all we need is a test case that has a function definition which has a comment after the closing parenthesis of the function header, e.g.:

void
foo() // this might be a function definition
{
}

[klimek]

+1 to quite some overlap in the tests. I'd have expected three test cases:

1. ; line comment
2. ; C-comment
3. no semi

[MyDeveloperDay]

In D60363#1458034, @klimek wrote:

+1 to quite some overlap in the tests. I'd have expected three test cases:

1. ; line comment
2. ; C-comment
3. no semi

Yes, sorry about that.. the tests were left over from my debugging investigation, I originally thought the bug was caused by the comment in the arguments and not by the comment after the ;, I'll update the patch to remove necessary test cases and resubmit

[MyDeveloperDay]

Addressing review comments
Reduce test cases

[owenpan]

clang/lib/Format/TokenAnnotator.h

114 ↗

(On Diff #194120)

I think endsWith() ignores tok::comment, so

return !endsWith(tok::semi);

should suffice?

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.h

114 ↗

(On Diff #194120)

I should ready the code comments more often! thanks, I'll update

[lebedev.ri]

Please either subscribe the correct lists or set the correct repo in differential params.

[MyDeveloperDay]

In D60363#1459297, @lebedev.ri wrote:

Please either subscribe the correct lists or set the correct repo in differential params.

Thanks for the correction, I was never really clear which one to use for the project as the blurb at the top of clang-tools-extra says "clang-format"

https://reviews.llvm.org/tag/clang-tools-extra/

But going forward I'll follow your example here.

[MyDeveloperDay]

use endsWith() as it ignored comments

[owenpan]

Looks good!

cfe/trunk/lib/Format/TokenAnnotator.h

public:
   template <typename... Ts> bool endsWith(Ts... Tokens) const {
     return Last && Last->endsSequence(Tokens...);
   }
 
   /// \c true if this line looks like a function definition instead of a
   /// function declaration. Asserts MightBeFunctionDecl.
   bool mightBeFunctionDefinition() const {
     assert(MightBeFunctionDecl);
-    // FIXME: Line.Last points to other characters than tok::semi
-    // and tok::lbrace.
-    return !Last->isOneOf(tok::semi, tok::comment);
+    // Try to determine if the end of a stream of tokens is either the
+    // Definition or the Declaration for a function. It does this by looking for
+    // the ';' in foo(); and using that it ends with a ; to know this is the
+    // Definition, however the line could end with
+    //    foo(); /* comment */
+    // or
+    //    foo(); // comment
+    // or
+    //    foo() // comment
+    // endsWith() ignores the comment.
+    return !endsWith(tok::semi);
   }
 
   /// \c true if this line starts a namespace definition.
   bool startsWithNamespace() const {
     return startsWith(tok::kw_namespace) ||
            startsWith(tok::kw_inline, tok::kw_namespace) ||
            startsWith(tok::kw_export, tok::kw_namespace);
   }

cfe/trunk/unittests/Format/FormatTest.cpp

verifyFormat("int\n"
                "}\n",
                Style);
   verifyFormat("int\n"
                "foo(A<B<8>, 8> a)\n"
                "{\n"
                "  return a;\n"
                "}\n",
                Style);
+
+  Style = getGNUStyle();
+
+  // Test for comments at the end of function declarations.
+  verifyFormat("void\n"
+               "foo (int a, /*abc*/ int b) // def\n"
+               "{\n"
+               "}\n",
+               Style);
+
+  verifyFormat("void\n"
+               "foo (int a, /* abc */ int b) /* def */\n"
+               "{\n"
+               "}\n",
+               Style);
+
+  // Definitions that should not break after return type
+  verifyFormat("void foo (int a, int b); // def\n", Style);
+  verifyFormat("void foo (int a, int b); /* def */\n", Style);
+  verifyFormat("void foo (int a, int b);\n", Style);
 }
 
 TEST_F(FormatTest, AlwaysBreakBeforeMultilineStrings) {
   FormatStyle NoBreak = getLLVMStyle();
   NoBreak.AlwaysBreakBeforeMultilineStrings = false;
   FormatStyle Break = getLLVMStyle();
   Break.AlwaysBreakBeforeMultilineStrings = true;
   verifyFormat("aaaa = \"bbbb\"\n"