ClangFormat - When adding a comment at the end of a return type declaration, the function name is incorrectly indented

[Abpostelnicu]

Summary

This addresses the problem that when having a comment at the end of a return type declaration the function name is incorrectly indented.
The bellow example, having this input:

template<typename T> // Templates on own line.
static int           // Return type on own line for top-level functions.
  MyFunction(int a)

The result would be:

template<typename T> // Templates on own line.
static int           // Return type on own line for top-level functions.
  MyFunction(int a)

The above result is wrong the correct one is:

template<typename T> // Templates on own line.
static int           // Return type on own line for top-level functions.
MyFunction(int a)

This was happening because in function isStartOfName the possibility oh having a comment before the actual name was not considered thus the comment was not ignored, like the const keyword is.

Test case will follow.

[Abpostelnicu]

test attached

[djasper]

Thanks for catching and fixing this.
Generally, please upload diffs with the full file as context. That way, phabricator allows expanding the code around your changes.

lib/Format/TokenAnnotator.cpp

1170 ↗

(On Diff #90688)

Just do:

FormatToken *PreviousNotConst = Tok.getPreviousNonComment();
while (PreviousNotConst && PreviousNotConst->is(tok::kw_const))
  PreviousNotConst = PreviousNotConst.getPreviousNonComment();

No Other changes should be necessary.

unittests/Format/FormatTest.cpp

8342 ↗

(On Diff #90688)

Please make the comment shorter so it doesn't need to be wrapped.

Also, put this into the test function where the rest of this functionality is covered, probably BreaksLongDeclarations.

[Abpostelnicu]

In D30646#693231, @djasper wrote:

Thanks for catching and fixing this.
Generally, please upload diffs with the full file as context. That way, phabricator allows expanding the code around your changes.

lib/Format/TokenAnnotator.cpp

1170 ↗

(On Diff #90688)

You are right but I've specially wanted to make things clearer :).

[djasper]

lib/Format/TokenAnnotator.cpp

1170 ↗

(On Diff #90688)

I see, I think it doesn't help much. I think comments should almost always be ignored and we shouldn't even need to make this explicit in the code. I have even thought ways to refactor the code to completely ignore them by default and only look at them in the few very specific cases where we do care.

[Abpostelnicu]

lib/Format/TokenAnnotator.cpp

1170 ↗

(On Diff #90688)

Ok I see your point there. I'll re update the patch.

[djasper]

Looks good, thank you!

[krasimir]

Thank you for the fix!

[Abpostelnicu]

Pushed a new change since the test case was broken, and broke the build.

cfe/trunk/lib/Format/TokenAnnotator.cpp

bool isStartOfName(const FormatToken &Tok) {
     if (Tok.Previous->isOneOf(TT_LeadingJavaAnnotation, Keywords.kw_instanceof,
                               Keywords.kw_as))
       return false;
     if (Style.Language == FormatStyle::LK_JavaScript &&
         Tok.Previous->is(Keywords.kw_in))
       return false;
 
     // Skip "const" as it does not have an influence on whether this is a name.
-    FormatToken *PreviousNotConst = Tok.Previous;
+    FormatToken *PreviousNotConst = Tok.getPreviousNonComment();
     while (PreviousNotConst && PreviousNotConst->is(tok::kw_const))
-      PreviousNotConst = PreviousNotConst->Previous;
+      PreviousNotConst = PreviousNotConst->getPreviousNonComment();
 
     if (!PreviousNotConst)
       return false;
 
     bool IsPPKeyword = PreviousNotConst->is(tok::identifier) &&
                        PreviousNotConst->Previous &&
                        PreviousNotConst->Previous->is(tok::hash);
 

cfe/trunk/unittests/Format/FormatTest.cpp

verifyFormat("typedef size_t (*aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa)(\n"
                "        *aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);");
   verifyFormat("void aaaaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
                "    vector<aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa>\n"
                "        aaaaaaaaaaaaaaaaaaaaaaaa);");
   verifyFormat("void aaaaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
                "    vector<aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa<\n"
                "        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa>>\n"
                "        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);");
+
+  verifyFormat("template<typename T> // Templates on own line.\n"
+               "static int           // Some comment.\n"
+               "MyFunction(int a);",
+               getLLVMStyle());
 }
 
 TEST_F(FormatTest, FormatsArrays) {
   verifyFormat("aaaaaaaaaaaaaaaaaaaaaaaaa[aaaaaaaaaaaaaaaaaaaaaaaaa]\n"
                "                         [bbbbbbbbbbbbbbbbbbbbbbbbb] = c;");
   verifyFormat("aaaaaaaaaaaaaaaaaaaaaaaaa[aaaaaaaaaaa(aaaaaaaaaaaa)]\n"
                "                         [bbbbbbbbbbb(bbbbbbbbbbbb)] = c;");
   verifyFormat("if (aaaaaaaaaaaaaaaaaaaaaaaa &&\n"