[clang-format] Propose a new solution to - Fix overlapping replacements before PPDirectives

[MyDeveloperDay]

Summary

Propose a new solution to

D151954: [clang-format] Fix overlapping whitespace replacements before PPDirective

That was reverted as part of

D152473: [clang-format] Add test case for issue 63170

[MyDeveloperDay]

ok this still doesn't resolve the original https://github.com/llvm/llvm-project/issues/62892

[MyDeveloperDay]

but it does seem to resolve https://github.com/llvm/llvm-project/issues/63170

[paulkirth]

Thanks for looking into this. LGTM from me. I'll keep an eye on our CI when this lands and let you know if we encounter any problems.

[MyDeveloperDay]

So whilst this solves the unit tests (and the golden.h issue) that were added it DOESN'T solve the overlapping replacements that was the original bug.

struct foo {
	void test() {
#if 1
#else
#endif
	}
#if 1
private:


#endif
};

This issue is caused by the private:\n\n\n and doesn't happen when there its only private:\n\n @owenpan , any thoughts, I'm abit stuck?

[owenpan]

I got something that looks promising:

diff --git a/clang/lib/Format/FormatToken.h b/clang/lib/Format/FormatToken.h
index dd23bd35411d..bba030238338 100644
--- a/clang/lib/Format/FormatToken.h
+++ b/clang/lib/Format/FormatToken.h
@@ -418,6 +418,12 @@ public:
   /// and thereby e.g. leave an empty line between two function definitions.
   unsigned NewlinesBefore = 0;

+  /// The number of newlines immediately before the \c Token after formatting.
+  ///
+  /// This is used to avoid overlapping whitespace replacements when \c Newlines
+  /// is recomputed for a finalized preprocessor branching directive.
+  int Newlines = -1;
+
   /// The offset just past the last '\n' in this token's leading
   /// whitespace (relative to \c WhiteSpaceStart). 0 if there is no '\n'.
   unsigned LastNewlineOffset = 0;
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 5172deb494b2..72293945122b 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -1418,22 +1418,13 @@ unsigned UnwrappedLineFormatter::format(
   return Penalty;
 }

-void UnwrappedLineFormatter::formatFirstToken(
-    const AnnotatedLine &Line, const AnnotatedLine *PreviousLine,
-    const AnnotatedLine *PrevPrevLine,
-    const SmallVectorImpl<AnnotatedLine *> &Lines, unsigned Indent,
-    unsigned NewlineIndent) {
-  FormatToken &RootToken = *Line.First;
-  if (RootToken.is(tok::eof)) {
-    unsigned Newlines =
-        std::min(RootToken.NewlinesBefore,
-                 Style.KeepEmptyLinesAtEOF ? Style.MaxEmptyLinesToKeep + 1 : 1);
-    unsigned TokenIndent = Newlines ? NewlineIndent : 0;
-    Whitespaces->replaceWhitespace(RootToken, Newlines, TokenIndent,
-                                   TokenIndent);
-    return;
-  }
-  unsigned Newlines =
+static auto computeNewlines(const AnnotatedLine &Line,
+                            const AnnotatedLine *PreviousLine,
+                            const AnnotatedLine *PrevPrevLine,
+                            const SmallVectorImpl<AnnotatedLine *> &Lines,
+                            const FormatStyle &Style) {
+  const auto &RootToken = *Line.First;
+  auto Newlines =
       std::min(RootToken.NewlinesBefore, Style.MaxEmptyLinesToKeep + 1);
   // Remove empty lines before "}" where applicable.
   if (RootToken.is(tok::r_brace) &&
@@ -1512,7 +1503,32 @@ void UnwrappedLineFormatter::formatFirstToken(
     }
   }

-  if (Newlines)
+  return Newlines;
+}
+
+void UnwrappedLineFormatter::formatFirstToken(
+    const AnnotatedLine &Line, const AnnotatedLine *PreviousLine,
+    const AnnotatedLine *PrevPrevLine,
+    const SmallVectorImpl<AnnotatedLine *> &Lines, unsigned Indent,
+    unsigned NewlineIndent) {
+  FormatToken &RootToken = *Line.First;
+  if (RootToken.is(tok::eof)) {
+    unsigned Newlines =
+        std::min(RootToken.NewlinesBefore,
+                 Style.KeepEmptyLinesAtEOF ? Style.MaxEmptyLinesToKeep + 1 : 1);
+    unsigned TokenIndent = Newlines ? NewlineIndent : 0;
+    Whitespaces->replaceWhitespace(RootToken, Newlines, TokenIndent,
+                                   TokenIndent);
+    return;
+  }
+
+  if (RootToken.Newlines < 0) {
+    RootToken.Newlines = computeNewlines(Line, PreviousLine, PrevPrevLine,
+                                         Lines, Style);
+  }
+
+  assert(RootToken.Newlines >= 0);
+  if (RootToken.Newlines > 0)
     Indent = NewlineIndent;

   // Preprocessor directives get indented before the hash only if specified. In
@@ -1524,7 +1540,7 @@ void UnwrappedLineFormatter::formatFirstToken(
     Indent = 0;
   }

-  Whitespaces->replaceWhitespace(RootToken, Newlines, Indent, Indent,
+  Whitespaces->replaceWhitespace(RootToken, RootToken.Newlines, Indent, Indent,
                                  /*IsAligned=*/false,
                                  Line.InPPDirective &&
                                      !RootToken.HasUnescapedNewline);

I'll submit a patch based on the above in a day or two.

[owenpan]

I've updated D151954. If that doesn't work, we should land this patch and continue to work on a more general solution.

clang/lib/Format/UnwrappedLineFormatter.cpp

case FormatStyle::ELBAMS_Always: {
       if ((!previousToken || !previousToken->is(tok::l_brace)) && Newlines <= 1)
         Newlines = 2;
     } break;
     }
   }
 
   // Insert or remove empty line after access specifiers.
   if (PreviousLine && PreviousLine->First->isAccessSpecifier() &&
+      !RootToken.Finalized &&
       (!PreviousLine->InPPDirective || !RootToken.HasUnescapedNewline)) {
     // EmptyLineBeforeAccessModifier is handling the case when two access
     // modifiers follow each other.
     if (!RootToken.isAccessSpecifier()) {
       switch (Style.EmptyLineAfterAccessModifier) {
       case FormatStyle::ELAAMS_Never:
         Newlines = 1;
         break;

clang/unittests/Format/FormatTest.cpp

verifyFormat("struct foo {\n"
                "private:\n"
                "\n"
                "#ifdef FOO\n"
                "#endif\n"
                "  void f() {}\n"
                "};\n",
                Style);
 
+  verifyFormat("struct foo {\n"
+               "#ifdef FOO1\n"
+               "#else\n"
+               "private:\n"
+               "\n"
+               "#endif\n"
+               "};",
+               "struct foo {\n"
+               "#ifdef FOO1\n"
+               "#else\n"
+               "private:\n"
+               "\n"
+               "#endif\n"
+               "};",
+               Style);
+
+  verifyFormat("struct foo {\n"
+               "#ifdef FOO2\n"
+               "#else\n"
+               "private:\n"
+               "\n"
+               "#endif\n"
+               "};",
+               "struct foo {\n"
+               "#ifdef FOO2\n"
+               "#else\n"
+               "private:\n"
+               "\n"
+               "\n"
+               "#endif\n"
+               "};",
+               Style);
+
   Style.EmptyLineAfterAccessModifier = FormatStyle::ELAAMS_Always;
   verifyFormat("struct foo {\n"
                "private:\n"
                "\n"
                "#ifdef FOO\n"
                "#endif\n"
                "  void f() {}\n"
                "};\n",