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

[Paul Hoad via cfe-commits]

Author: paulhoad
Date: Mon Apr 15 00:47:15 2019
New Revision: 358375

URL: http://llvm.org/viewvc/llvm-project?rev=358375&view=rev
Log:
[clang-format] [PR41170] Break after return type ignored with certain comments positions

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>`

Reviewers: klimek, djasper, reuk, russellmcc, owenpan, sammccall
Reviewed By: owenpan
Subscribers: lebedev.ri, cfe-commits, sammccall
Tags: #clang
Differential Revision: https://reviews.llvm.org/D60363

Modified:
    cfe/trunk/lib/Format/TokenAnnotator.h
    cfe/trunk/unittests/Format/FormatTest.cpp

Modified: cfe/trunk/lib/Format/TokenAnnotator.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/TokenAnnotator.h?rev=358375&r1=358374&r2=358375&view=diff
==============================================================================
--- cfe/trunk/lib/Format/TokenAnnotator.h (original)
+++ cfe/trunk/lib/Format/TokenAnnotator.h Mon Apr 15 00:47:15 2019
@@ -99,9 +99,17 @@ public:
   /// 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.

Modified: cfe/trunk/unittests/Format/FormatTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=358375&r1=358374&r2=358375&view=diff
==============================================================================
--- cfe/trunk/unittests/Format/FormatTest.cpp (original)
+++ cfe/trunk/unittests/Format/FormatTest.cpp Mon Apr 15 00:47:15 2019
@@ -5768,6 +5768,26 @@ TEST_F(FormatTest, ReturnTypeBreakingSty
                "  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) {