[clang-format] Fix bug with ENAS_DontAlign and empty lines

[jtbandes]

Summary

This fixes a bug in ENAS_DontAlign (introduced in D32733) where blank lines had an EscapedNewlineColumn of 0, causing a subtraction to overflow when converted back to unsigned and leading to runaway memory allocation.

This restores the original approach of a separate loop as originally proposed in https://reviews.llvm.org/D32733?vs=97397&id=97404, now with a proper justification :)

[djasper]

Could you explain this in more detail? Which subtraction is underflowing? Why can't we just add a ternary expression there to handle the case?

[jtbandes]

I can add some clarity but I can't claim to fully understand the whole program flow here yet, so my explanation is probably insufficient.

The overflow (underflow? but I think that means something specific to FP) is on line formerly-650: EscapedNewlineColumn - Offset - 1. This expression computes number of spaces required to place the \ in the correct position. When EscapedNewlineColumn was 0, the Offset would end up being large enough for the computation on line 650 to roll over to a large value.

Here is where my understanding is a bit fuzzier, and perhaps I should spend more time reading the code. When the alignment style is not ENAS_DontAlign, the function alignEscapedNewlines() adjusts the EscapedNewlineColumn of each line, including blank lines, which seems to result in EscapedNewlineColumn being always sufficiently large to do this subtraction without wrapping. (The approach of immediately return;ing for ENAS_DontAlign, committed in rL302428, was causing the EscapedNewlineColumn to be 0 here.)

It is a possibility to simply check the EscapedNewlineColumn and do something different inside of appendNewlineText(). I'd be interested to hear your thoughts on which approach is better. Perhaps if I study the code some more there will a nice simplification will reveal itself to me...

[jtbandes]

Okay, I think this approach is better:

• Rename the version of appendNewlineText used for escaped newlines to appendEscapedNewlineText to reduce confusability.
• If ENAS_DontAlign, skip all of the offset calculation logic. Just append space-backslash-newline.
• Restore the offset calculation to use EscapedNewlineColumn - 1, which it was before D32733. This was only changed to support the early-return approach.
• Leave in the assert.

[jtbandes]

• Undo change in argument list

[jtbandes]

@djasper Bump :)

[djasper]

lib/Format/WhitespaceManager.cpp

650 ↗

(On Diff #108863)

Note that when you have an empty line, this would turn into:

#define A \
  int i; \
 \                <-- Note the 1-space indent here.
  int j; \
  int k;

With my alternative below, that "\" will just be put at column 0, which probably isn't better or worse.

656 ↗

(On Diff #108863)

You could change this to:

unsigned Spaces =
    std::max<int>(1, EscapedNewlineColumn - PreviousEndOfTokenColumn - 1);
for (unsigned i = 0; i < Newlines; ++i) {
  Text.append(Spaces, ' ');
  Text.append(UseCRLF ? "\\\r\n" : "\\\n");
  Spaces = std::max<int>(0, EscapedNewlineColumn - 1);
}

And it should work without problems and without special code path.

[jtbandes]

@djasper ok, done

[jtbandes]

lib/Format/WhitespaceManager.cpp

650 ↗

(On Diff #108863)

I suppose that can be changed back to 1 by using std::max<int>(1, EscapedNewlineColumn - 1); instead, right? I don't have strong feelings about whether it should be 0 or 1.

[djasper]

Thanks you.

lib/Format/WhitespaceManager.cpp

650 ↗

(On Diff #108863)

I don't either. Leave as is for now.

[jtbandes]

Thanks. Can you commit this when you get a chance? I don't have permissions.

cfe/trunk/lib/Format/WhitespaceManager.h

private:
   void alignEscapedNewlines(unsigned Start, unsigned End, unsigned Column);
 
   /// \brief Fill \c Replaces with the replacements for all effective changes.
   void generateChanges();
 
   /// \brief Stores \p Text as the replacement for the whitespace in \p Range.
   void storeReplacement(SourceRange Range, StringRef Text);
   void appendNewlineText(std::string &Text, unsigned Newlines);
-  void appendNewlineText(std::string &Text, unsigned Newlines,
+  void appendEscapedNewlineText(std::string &Text, unsigned Newlines,
                                 unsigned PreviousEndOfTokenColumn,
                                 unsigned EscapedNewlineColumn);
   void appendIndentText(std::string &Text, unsigned IndentLevel,
                         unsigned Spaces, unsigned WhitespaceStartColumn);
 
   SmallVector<Change, 16> Changes;
   const SourceManager &SourceMgr;
   tooling::Replacements Replaces;
   const FormatStyle &Style;
   bool UseCRLF;
 };
 
 } // namespace format
 } // namespace clang
 
 #endif

cfe/trunk/lib/Format/WhitespaceManager.cpp

for (unsigned i = 0, e = Changes.size(); i != e; ++i) {
     if (i > 0) {
       assert(Changes[i - 1].OriginalWhitespaceRange.getBegin() !=
                  C.OriginalWhitespaceRange.getBegin() &&
              "Generating two replacements for the same location");
     }
     if (C.CreateReplacement) {
       std::string ReplacementText = C.PreviousLinePostfix;
       if (C.ContinuesPPDirective)
-        appendNewlineText(ReplacementText, C.NewlinesBefore,
-                          C.PreviousEndOfTokenColumn, C.EscapedNewlineColumn);
+        appendEscapedNewlineText(ReplacementText, C.NewlinesBefore,
+                                 C.PreviousEndOfTokenColumn,
+                                 C.EscapedNewlineColumn);
       else
         appendNewlineText(ReplacementText, C.NewlinesBefore);
       appendIndentText(ReplacementText, C.Tok->IndentLevel,
                        std::max(0, C.Spaces),
                        C.StartOfTokenColumn - std::max(0, C.Spaces));
       ReplacementText.append(C.CurrentLinePrefix);
       storeReplacement(C.OriginalWhitespaceRange, ReplacementText);
     }

 }
 
 void WhitespaceManager::appendNewlineText(std::string &Text,
                                           unsigned Newlines) {
   for (unsigned i = 0; i < Newlines; ++i)
     Text.append(UseCRLF ? "\r\n" : "\n");
 }
 
-void WhitespaceManager::appendNewlineText(std::string &Text, unsigned Newlines,
+void WhitespaceManager::appendEscapedNewlineText(std::string &Text,
+                                                 unsigned Newlines,
                                                  unsigned PreviousEndOfTokenColumn,
                                                  unsigned EscapedNewlineColumn) {
   if (Newlines > 0) {
-    unsigned Offset =
-        std::min<int>(EscapedNewlineColumn - 2, PreviousEndOfTokenColumn);
+    unsigned Spaces =
+        std::max<int>(1, EscapedNewlineColumn - PreviousEndOfTokenColumn - 1);
     for (unsigned i = 0; i < Newlines; ++i) {
-      Text.append(EscapedNewlineColumn - Offset - 1, ' ');
+      Text.append(Spaces, ' ');
       Text.append(UseCRLF ? "\\\r\n" : "\\\n");
-      Offset = 0;
+      Spaces = std::max<int>(0, EscapedNewlineColumn - 1);
     }
   }
 }
 
 void WhitespaceManager::appendIndentText(std::string &Text,
                                          unsigned IndentLevel, unsigned Spaces,
                                          unsigned WhitespaceStartColumn) {
   switch (Style.UseTab) {

cfe/trunk/unittests/Format/FormatTest.cpp

 TEST_F(FormatTest, EscapedNewlines) {
   EXPECT_EQ(
       "#define A \\\n  int i;  \\\n  int j;",
       format("#define A \\\nint i;\\\n  int j;", getLLVMStyleWithColumns(11)));
   EXPECT_EQ("#define A\n\nint i;", format("#define A \\\n\n int i;"));
   EXPECT_EQ("template <class T> f();", format("\\\ntemplate <class T> f();"));
   EXPECT_EQ("/* \\  \\  \\\n */", format("\\\n/* \\  \\  \\\n */"));
   EXPECT_EQ("<a\n\\\\\n>", format("<a\n\\\\\n>"));
+
+  FormatStyle DontAlign = getLLVMStyle();
+  DontAlign.AlignEscapedNewlines = FormatStyle::ENAS_DontAlign;
+  DontAlign.MaxEmptyLinesToKeep = 3;
+  // FIXME: can't use verifyFormat here because the newline before
+  // "public:" is not inserted the first time it's reformatted
+  EXPECT_EQ("#define A \\\n"
+            "  class Foo { \\\n"
+            "    void bar(); \\\n"
+            "\\\n"
+            "\\\n"
+            "\\\n"
+            "  public: \\\n"
+            "    void baz(); \\\n"
+            "  };",
+            format("#define A \\\n"
+                   "  class Foo { \\\n"
+                   "    void bar(); \\\n"
+                   "\\\n"
+                   "\\\n"
+                   "\\\n"
+                   "  public: \\\n"
+                   "    void baz(); \\\n"
+                   "  };", DontAlign));
 }
 
 TEST_F(FormatTest, CalculateSpaceOnConsecutiveLinesInMacro) {
   verifyFormat("#define A \\\n"
                "  int v(  \\\n"
                "      a); \\\n"
                "  int i;",
                getLLVMStyleWithColumns(11));