[clang-format] Keep comments aligned to macros

[mzeren-vmw]

Summary

r312125, which introduced preprocessor indentation, shipped with a known
issue where "indentation of comments immediately before indented
preprocessor lines is toggled on each run". For example these two forms
toggle:

#ifndef HEADER_H
#define HEADER_H
#if 1
// comment
#   define A 0
#endif
#endif

#ifndef HEADER_H
#define HEADER_H
#if 1
   // comment
#   define A 0
#endif
#endif

This happens because we check vertical alignment against the "#" yet
indent to the level of the "define". This patch resolves this issue by
checking vertical alignment against the "define", and by tracking a
"LevelOffset" (0 or 1) in each AnnotatedLine to account for the
off-by-one indentation of preprocessor lines.

[klimek]

Just from a formatting point of view, why not:

//.   Comment
#.    define X

[mzeren-vmw]

In D42036#976827, @klimek wrote:

Just from a formatting point of view, why not:

//.   Comment
#.    define X

(assuming the '.'s are unintentional)
There is some logic in placing // in column 0, since we place # in column 0. However, we do not have examples of that style in our code base. We do have examples of aligning // above define:

   // Comment
#  define

Are you suggesting that c-f manage the indent after the // (or ///, etc.)? This seems more complex than managing the space between # and <directive>. I do want c-f to be able to re-indent aligned comments if an #if is inserted above.

[krasimir]

lib/Format/TokenAnnotator.cpp

1710

Please comment these.

1756

This feels a bit awkward: we're adding code that implicitly assumes the exact style the preprocessor directives and comments around them are handled. Maybe if this could become part of the level itself, it would feel less awkward.

lib/Format/TokenAnnotator.h

41

Is there a way to not introduce LevelOffset, but have it part of Level?

unittests/Format/FormatTest.cpp

2619

I would like to see test including multiline //-comment sections before, inside and after preprocessor directives as well as /**/-style comments.

[mzeren-vmw]

Documented CommentAlignment enumerators. Documenting them suggested better
enumerator names.

Added tests for multi-line comments, block comments and trailing comments.

[mzeren-vmw]

lib/Format/TokenAnnotator.cpp

1756

I agree that the "long distance coupling" is awkward. Perhaps the new enumerator names make this a bit more palatable?

lib/Format/TokenAnnotator.h

41

Level is an abstract indentation level, while LevelOffset is "columns". They have different units. Maybe it would be feasible to change the units of "Level" to columns in order to merge these two variables, but doing so would throw away information. It also seems like a much larger change. We could create a composite type class AnnotatedLevel { private: unsigned Level, unsigned Offset public: <strongly typed operations> } but that seems over-engineered. Any other ideas?

[krasimir]

lib/Format/TokenAnnotator.cpp

1725

Why are we checking NextNonComment.Level > 0 here? We could be aligned with the next preprocessor directive even at level 0.

1729

I think this should be enabled only if preprocessor indentation has been enabled.

1756

Could you add a comment that this 1 comes from the # in the preprocessor directive?

unittests/Format/FormatTest.cpp

2629

I would expect this comment to be aligned with the #endif on the next line.

2657

Same here.

[djasper]

While I agree that there is probably a bug to fix, I don't (yet) agree with what is proposed in this patch. I think a comment in between preprocessor directives should always either:

• Be considered part of the code in between the #-lines
• Be considered to be commenting on the subsequent #-line

In the former case, we need to indent with the regular IndentWidth, completely irrespective of anything inside the preprocessor lines. In the latter case, we should align with the # in column 0. To me, aligning with the define seems fundamentally wrong.

[mzeren-vmw]

In D42036#984401, @djasper wrote:

To me, aligning with the define seems fundamentally wrong.

we definitely have code that does that internally. It can also be seen in the wild e.g.:
https://github.com/boostorg/config/blob/develop/include/boost/config/detail/posix_features.hpp
However, it seems reasonable that clang-format's "default" be alignment with #. That will be a simpler patch, and it will resolve the toggling behavior. Let me work that up in a separate review.

lib/Format/TokenAnnotator.h

enum LineType {
   LT_Other,
   LT_PreprocessorDirective,
   LT_VirtualFunctionDecl
 };
 
 class AnnotatedLine {
 public:
   AnnotatedLine(const UnwrappedLine &Line)
-      : First(Line.Tokens.front().Tok), Level(Line.Level),
+      : First(Line.Tokens.front().Tok), Level(Line.Level), LevelOffset(0),
         MatchingOpeningBlockLineIndex(Line.MatchingOpeningBlockLineIndex),
         InPPDirective(Line.InPPDirective),
         MustBeDeclaration(Line.MustBeDeclaration), MightBeFunctionDecl(false),
         IsMultiVariableDeclStmt(false), Affected(false),
         LeadingEmptyLinesAffected(false), ChildrenAffected(false),
         FirstStartColumn(Line.FirstStartColumn) {
     assert(!Line.Tokens.empty());
 
public:
 
   FormatToken *First;
   FormatToken *Last;
 
   SmallVector<AnnotatedLine *, 0> Children;
 
   LineType Type;
   unsigned Level;
+
+  /// Adjustment to Level based indent. When comments are aligned to the next
+  /// preprocessor line they must use the same offset as the directive,
+  /// typically 1 due to the leading #.
+  unsigned LevelOffset;
+
   size_t MatchingOpeningBlockLineIndex;
   bool InPPDirective;
   bool MustBeDeclaration;
   bool MightBeFunctionDecl;
   bool IsMultiVariableDeclStmt;
 
   /// \c True if this line should be formatted, i.e. intersects directly or
   /// indirectly with one of the input ranges.

lib/Format/TokenAnnotator.cpp

while (Current &&
       Current = Current->Next;
   }
 
   const FormatStyle &Style;
   const AdditionalKeywords &Keywords;
   FormatToken *Current;
 };
 
+enum CommentAlignment { CA_None, CA_Code, CA_Preprocessor };
+
+CommentAlignment getCommentAlignment(const AnnotatedLine &Comment,
+                                     const AnnotatedLine &NextNonComment) {
+  if (NextNonComment.First->NewlinesBefore > 1)
+    return CA_None;
+  // If the next line is an indented preprocessor line look at the directive,
+  // not the #.
+  if ((NextNonComment.Type == LT_PreprocessorDirective ||
+       NextNonComment.Type == LT_ImportStatement) &&
+      NextNonComment.Level > 0 && !Comment.InPPDirective)
+    return NextNonComment.First->Next &&
+                   (Comment.First->OriginalColumn ==
+                    NextNonComment.First->Next->OriginalColumn)
+               ? CA_Preprocessor
+               : CA_None;
+  else
+    return Comment.First->OriginalColumn == NextNonComment.First->OriginalColumn
+               ? CA_Code
+               : CA_None;
+}
+
 } // end anonymous namespace
 
 void TokenAnnotator::setCommentLineLevels(
     SmallVectorImpl<AnnotatedLine *> &Lines) {
   const AnnotatedLine *NextNonCommentLine = nullptr;
   for (SmallVectorImpl<AnnotatedLine *>::reverse_iterator I = Lines.rbegin(),
                                                           E = Lines.rend();
        I != E; ++I) {
     bool CommentLine = true;
     for (const FormatToken *Tok = (*I)->First; Tok; Tok = Tok->Next) {
       if (!Tok->is(tok::comment)) {
         CommentLine = false;
         break;
       }
     }
 
-    if (NextNonCommentLine && CommentLine) {
-      // If the comment is currently aligned with the line immediately following
-      // it, that's probably intentional and we should keep it.
-      bool AlignedWithNextLine =
-          NextNonCommentLine->First->NewlinesBefore <= 1 &&
-          NextNonCommentLine->First->OriginalColumn ==
-              (*I)->First->OriginalColumn;
-      if (AlignedWithNextLine)
+    // If the comment is aligned with the line immediately following it, that's
+    // probably intentional and we should keep it.
+    if (CommentLine && NextNonCommentLine) {
+      CommentAlignment Alignment =
+          getCommentAlignment(**I, *NextNonCommentLine);
+      if (Alignment != CA_None)
         (*I)->Level = NextNonCommentLine->Level;
+      if (Alignment == CA_Preprocessor)
+        (*I)->LevelOffset = 1;
     } else {
       NextNonCommentLine = (*I)->First->isNot(tok::r_brace) ? (*I) : nullptr;
     }
 
     setCommentLineLevels((*I)->Children);
   }
 }
 

lib/Format/UnwrappedLineFormatter.cpp

public:
 
   /// \brief Returns the indent for the current line.
   unsigned getIndent() const { return Indent; }
 
   /// \brief Update the indent state given that \p Line is going to be formatted
   /// next.
   void nextLine(const AnnotatedLine &Line) {
     Offset = getIndentOffset(*Line.First);
+    Offset += Line.LevelOffset;
     // Update the indent level cache size so that we can rely on it
     // having the right size in adjustToUnmodifiedline.
     while (IndentForLevel.size() <= Line.Level)
       IndentForLevel.push_back(-1);
     if (Line.InPPDirective) {
       Indent = Line.Level * Style.IndentWidth + AdditionalIndent;
     } else {
       IndentForLevel.resize(Line.Level + 1);

unittests/Format/FormatTest.cpp

verifyFormat("#ifndef HEADER_H\n"
                Style);
   // Include guards must have a #define with the same variable immediately
   // after #ifndef.
   verifyFormat("#ifndef NOT_GUARD\n"
                "#  define FOO\n"
                "code();\n"
                "#endif",
                Style);
-
   // Include guards must cover the entire file.
   verifyFormat("code();\n"
                "code();\n"
                "#ifndef NOT_GUARD\n"
                "#  define NOT_GUARD\n"
                "code();\n"
                "#endif",
                Style);
EXPECT_EQ("code();\n"
             "code();\n"
             "#endif",
             format("code();\n"
                    "#ifndef HEADER_H\n"
                    "#  define HEADER_H\n"
                    "code();\n"
                    "#endif",
                    Style));
-  // FIXME: The comment indent corrector in TokenAnnotator gets thrown off by
-  // preprocessor indentation.
-  EXPECT_EQ("#if 1\n"
-            "  // comment\n"
+  // Comments aligned to macros stay aligned. This test is incompatible with
+  // verifyFormat() because messUp() removes the alignment.
+  {
+    const char *Expected = "// Level 0 unaligned comment\n"
+                           "#ifndef HEADER_H\n"
+                           "// Level 0 aligned comment\n"
+                           "#define HEADER_H\n"
+                           "#if 1\n"
+                           "   // aligned comment\n"
                            "#  define A 0\n"
-            "// comment\n"
+                           "// un-aligned comment\n"
                            "#  define B 0\n"
-            "#endif",
-            format("#if 1\n"
-                   "// comment\n"
+                           "#endif\n"
+                           "#endif";
+    const char *ToFormat = " // Level 0 unaligned comment\n"
+                           "#ifndef HEADER_H\n"
+                           "// Level 0 aligned comment\n"
+                           "#define HEADER_H\n"
+                           "#if 1\n"
+                           "   // aligned comment\n"
                            "#  define A 0\n"
-                   "   // comment\n"
+                           "  // un-aligned comment\n"
                            "#  define B 0\n"
-                   "#endif",
-                   Style));
+                           "#endif\n"
+                           "#endif";
+    EXPECT_EQ(Expected, format(ToFormat, Style));
+    EXPECT_EQ(Expected, format(Expected, Style));
+  }
   // Test with tabs.
   Style.UseTab = FormatStyle::UT_Always;
   Style.IndentWidth = 8;
   Style.TabWidth = 8;
   verifyFormat("#ifdef _WIN32\n"
                "#\tdefine A 0\n"
                "#\tifdef VAR2\n"
                "#\t\tdefine B 1\n"
                "#\t\tinclude <someheader.h>\n"
                "#\t\tdefine MACRO          \\\n"
                "\t\t\tsome_very_long_func_aaaaaaaaaa();\n"
                "#\tendif\n"
                "#else\n"
                "#\tdefine A 1\n"
                "#endif",

 
 TEST_F(FormatTest, FormatHashIfNotAtStartOfLine) {
   verifyFormat("{\n  { a #c; }\n}");
 }
 
 TEST_F(FormatTest, FormatUnbalancedStructuralElements) {
   EXPECT_EQ("#define A \\\n  {       \\\n    {\nint i;",
             format("#define A { {\nint i;", getLLVMStyleWithColumns(11)));
   EXPECT_EQ("#define A \\\n  }       \\\n  }\nint i;",
             format("#define A } }\nint i;", getLLVMStyleWithColumns(11)));
 }
 
 TEST_F(FormatTest, EscapedNewlines) {
   FormatStyle Narrow = getLLVMStyleWithColumns(11);
   EXPECT_EQ("#define A \\\n  int i;  \\\n  int j;",
             format("#define A \\\nint i;\\\n  int j;", Narrow));
   EXPECT_EQ("#define A\n\nint i;", format("#define A \\\n\n int i;"));