Fix issue: clang-format result is not consistent if "// clang-format off" is followed by macro definition.

[owenpan]

Summary

For example, the above code:

void main()
{
    // clang-format off
    #define Sum(x, y) ((x) + (y))
    Sum(1, 2);
    #undef Sum
    // clang-format on
}

If we run clang-format we will get the following result:

void main()
{
// clang-format off
    #define Sum(x, y) ((x) + (y))
    Sum(1, 2);
    #undef Sum
    // clang-format on
}

But if we run clang-format again, we will get another result:

void main()
{
    // clang-format off
    #define Sum(x, y) ((x) + (y))
    Sum(1, 2);
    #undef Sum
    // clang-format on
}

I think the expectation should be "no mater how many times clang-format runs, the result should be the same."

This patch tries to fix this issue.

[HazardyKnusperkeks]

Is this still an issue?

[owenpan]

This is no longer an issue in version 17, e.g. 7a4cdbe:

$ clang-format
void main()
{
    // clang-format off
    #define Sum(x, y) ((x) + (y))
    Sum(1, 2);
    #undef Sum
    // clang-format on
}
void main() {
  // clang-format off
    #define Sum(x, y) ((x) + (y))
    Sum(1, 2);
    #undef Sum
  // clang-format on
}

[MyDeveloperDay]

@owenpan @HazardyKnusperkeks @rymiel what are your feeling on how we should close old clang-format reviews like this? (@aaron.ballman any thoughts on how it should be done?)

we end up with lots of reviews that either get metaphorically abandoned, (without actually being abandoned).. should we have some sort of rule, that says reviews left for N months with no activity are automatically abandoned (to do this you have to comindeer the revision, which I don't like doing)

Part of me just wants to consume the unit tests (if they pass).. do you think we should do anything? what about issues we effectively are saying "no to" like D147969 but are then not abandoned by the author?

[aaron.ballman]

In D89918#4460401, @MyDeveloperDay wrote:

@owenpan @HazardyKnusperkeks @rymiel what are your feeling on how we should close old clang-format reviews like this? (@aaron.ballman any thoughts on how it should be done?)

we end up with lots of reviews that either get metaphorically abandoned, (without actually being abandoned).. should we have some sort of rule, that says reviews left for N months with no activity are automatically abandoned (to do this you have to comindeer the revision, which I don't like doing)

Part of me just wants to consume the unit tests (if they pass).. do you think we should do anything? what about issues we effectively are saying "no to" like D147969 but are then not abandoned by the author?

I don't think we have any sort of community policy on how to handle this, it's basically a case by case basis. If someone submits a patch and then (explicitly or implicitly) abandons the work, I believe it is acceptable to grab changes of value out of the patch and apply them to the code base as you see fit (the user contributing the patch expected their changes to make it in to the project, so I doubt anyone would be upset in practice). I would recommend linking to the review where the code originated from when committing the changes so there's good attribution for the commit.

To handle the review itself, you can commandeer the patch to abandon it or you can resign as a reviewer from the revision, but that's about the only two ways I know of to address that. Both approaches have social pressure (commandeering or resigning are not the easiest decisions for folks to make), so my guess is this will be inconsistently handled which suggests that resigning from the revision is the better approach to prefer as that gets you the work queue you want without requiring others to play along consistently. But I think either approach is acceptable -- I've seen folks do it both ways.

[owenpan]

Regarding commandeering, I would wait for a couple of weeks after pinging the author.

[HazardyKnusperkeks]

Give the author some time is okay, but I think we can and should commandeer at some point and abandon.

clang/lib/Format/TokenAnnotator.cpp

for (SmallVectorImpl<AnnotatedLine *>::reverse_iterator I = Lines.rbegin(),
     // it, that's probably intentional and we should keep it.
     if (NextNonCommentLine && CommentLine &&
         NextNonCommentLine->First->NewlinesBefore <= 1 &&
         NextNonCommentLine->First->OriginalColumn ==
             (*I)->First->OriginalColumn) {
       // Align comments for preprocessor lines with the # in column 0 if
       // preprocessor lines are not indented. Otherwise, align with the next
       // line.
+      if ((*I)->First->TokenText != "// clang-format off" &&
+          (*I)->First->TokenText != "/* clang-format off */") {
         (*I)->Level =
             (Style.IndentPPDirectives != FormatStyle::PPDIS_BeforeHash &&
              (NextNonCommentLine->Type == LT_PreprocessorDirective ||
               NextNonCommentLine->Type == LT_ImportStatement))
                 ? 0
                 : NextNonCommentLine->Level;
+      }
     } else {
       NextNonCommentLine = (*I)->First->isNot(tok::r_brace) ? (*I) : nullptr;
     }
 
     setCommentLineLevels((*I)->Children);
   }
 }
 

clang/unittests/Format/FormatTest.cpp

 }
 TEST_F(FormatTest, WebKitDefaultStyle) {
   FormatStyle Style = getWebKitStyle();
   verifyFormat("extern \"C\" {\n"
                "int foo();\n"
                "}",
                Style);
 }
+
+TEST_F(FormatTest, FormatTurnOffFollowedByMacro1) {
+  EXPECT_EQ("void main() {\n"
+            "  // clang-format off\n"
+            "  #define Sum(x, y) ((x) + (y))\n"
+            "  Sum(1, 2);\n"
+            "  #undef Sum\n"
+            "  // clang-format on\n"
+            "};",
+            format("void main() {\n"
+                   "  // clang-format off\n"
+                   "  #define Sum(x, y) ((x) + (y))\n"
+                   "  Sum(1, 2);\n"
+                   "  #undef Sum\n"
+                   "  // clang-format on\n"
+                   "};"));
+}
+
+TEST_F(FormatTest, FormatTurnOffFollowedByMacro2) {
+  EXPECT_EQ("void main() {\n"
+            "  // clang-format off\n"
+            "  #define Sum(x, y) ((x) + (y))\n"
+            "  Sum(1, 2);\n"
+            "  #undef Sum\n"
+            "  // clang-format on\n"
+            "};",
+            format("void main() {\n"
+                   "// clang-format off\n"
+                   "  #define Sum(x, y) ((x) + (y))\n"
+                   "  Sum(1, 2);\n"
+                   "  #undef Sum\n"
+                   "  // clang-format on\n"
+                   "};"));
+}
+
 } // namespace
 } // namespace format
 } // namespace clang