[clang-format] Fix overlapping whitespace replacements before PPDirective

[owenpan]

Summary

If the first token of an annotated line already has a computed Newlines, reuse it to avoid potential overlapping whitespace replacements before preprocessor branching directives.

Fixes https://github.com/llvm/llvm-project/issues/62892.

[owenpan]

clang/lib/Format/UnwrappedLineFormatter.cpp

1425

I will remove this parameter as it's Line.First.

[owenpan]

Removed the RootToken parameter.

[paulkirth]

This patch seems to introduce some significant regressions to formatting. I've filed https://github.com/llvm/llvm-project/issues/63170 with some candidate code. I'll try to reduce the case as I can, but I'm guessing something is incorrect w/ how the indentation level is tracked, since we see includes being incorrectly indented and lots of lines w/ whitespace.

[paulkirth]

I'd also suggest reverting this until it can be fixed

[owenpan]

In D151954#4403921, @paulkirth wrote:

This patch seems to introduce some significant regressions to formatting. I've filed https://github.com/llvm/llvm-project/issues/63170 with some candidate code. I'll try to reduce the case as I can, but I'm guessing something is incorrect w/ how the indentation level is tracked, since we see includes being incorrectly indented and lots of lines w/ whitespace.

https://github.com/llvm/llvm-project/issues/63170#issuecomment-1581331587

[thakis]

Looks like the revert broke tests: http://45.33.8.238/linux/109159/step_7.txt

[owenpan]

@leonardchan I'm not sure this patch should be reverted just yet. I can't reproduce the regression, and this patch does fix a previous regression.

[owenpan]

Add a member Newlines to struct FormatToken and compute it only once in UnwrappedLineFormatter::formatFirstToken().

[owenpan]

Rebased and changed a unit test to use verifyNoChange before landing.

clang/lib/Format/UnwrappedLineFormatter.cpp

for (const AnnotatedLine *Line =
     }
     if (!DryRun)
       markFinalized(TheLine.First);
   }
   PenaltyCache[CacheKey] = Penalty;
   return Penalty;
 }
 
-void UnwrappedLineFormatter::formatFirstToken(
-    const AnnotatedLine &Line, const AnnotatedLine *PreviousLine,
+static auto newlinesBeforeLine(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, 1u);
-    unsigned TokenIndent = Newlines ? NewlineIndent : 0;
-    Whitespaces->replaceWhitespace(RootToken, Newlines, TokenIndent,
-                                   TokenIndent);
-    return;
-  }
+                               const SmallVectorImpl<AnnotatedLine *> &Lines,
+                               const FormatToken &RootToken,
+                               const FormatStyle &Style) {
   unsigned Newlines =
       std::min(RootToken.NewlinesBefore, Style.MaxEmptyLinesToKeep + 1);
   // Remove empty lines before "}" where applicable.
   if (RootToken.is(tok::r_brace) &&
       (!RootToken.Next ||
        (RootToken.Next->is(tok::semi) && !RootToken.Next->Next)) &&
       // Do not remove empty lines before namespace closing "}".
       !getNamespaceToken(&Line, Lines)) {
if (!RootToken.isAccessSpecifier()) {
           Newlines = 1u;
         else
           Newlines = std::max(Newlines, 2u);
         break;
       }
     }
   }
 
+  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, 1u);
+    unsigned TokenIndent = Newlines ? NewlineIndent : 0;
+    Whitespaces->replaceWhitespace(RootToken, Newlines, TokenIndent,
+                                   TokenIndent);
+    return;
+  }
+
+  const auto Newlines =
+      RootToken.Finalized ? RootToken.NewlinesBefore
+                          : newlinesBeforeLine(Line, PreviousLine, PrevPrevLine,
+                                               Lines, RootToken, Style);
   if (Newlines)
     Indent = NewlineIndent;
 
   // Preprocessor directives get indented before the hash only if specified. In
   // Javascript import statements are indented like normal statements.
   if (!Style.isJavaScript() &&
       Style.IndentPPDirectives != FormatStyle::PPDIS_BeforeHash &&
       (Line.Type == LT_PreprocessorDirective ||

clang/unittests/Format/FormatTest.cpp

verifyFormat("struct foo {\n"
                "struct foo {\n"
                "private:\n"
                "\n"
                "#ifdef FOO\n"
                "#endif\n"
                "  void f() {}\n"
                "};\n",
                Style);
+  verifyFormat("struct foo {\n"
+               "#ifdef FOO\n"
+               "#else\n"
+               "private:\n"
+               "\n"
+               "#endif\n"
+               "};",
+               "struct foo {\n"
+               "#ifdef FOO\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"