[PATCH] D36019: [clang-format] Fix bug with ENAS_DontAlign and empty lines
This revision was automatically updated to reflect the committed changes.
Closed by commit rL310539: clang-format: Fix bug with ENAS_DontAlign and empty
lines (authored by jtbandes).
Repository:
rL LLVM
https://reviews.llvm.org/D36019
Files:
cfe/trunk/lib/Format/WhitespaceManager.cpp
cfe/trunk/lib/Format/WhitespaceManager.h
cfe/trunk/unittests/Format/FormatTest.cpp
Index: cfe/trunk/lib/Format/WhitespaceManager.h
===
--- cfe/trunk/lib/Format/WhitespaceManager.h
+++ cfe/trunk/lib/Format/WhitespaceManager.h
@@ -195,9 +195,9 @@
/// \brief Stores \p Text as the replacement for the whitespace in \p Range.
void storeReplacement(SourceRange Range, StringRef Text);
void appendNewlineText(std::string , unsigned Newlines);
- void appendNewlineText(std::string , unsigned Newlines,
- unsigned PreviousEndOfTokenColumn,
- unsigned EscapedNewlineColumn);
+ void appendEscapedNewlineText(std::string , unsigned Newlines,
+unsigned PreviousEndOfTokenColumn,
+unsigned EscapedNewlineColumn);
void appendIndentText(std::string , unsigned IndentLevel,
unsigned Spaces, unsigned WhitespaceStartColumn);
Index: cfe/trunk/lib/Format/WhitespaceManager.cpp
===
--- cfe/trunk/lib/Format/WhitespaceManager.cpp
+++ cfe/trunk/lib/Format/WhitespaceManager.cpp
@@ -603,8 +603,9 @@
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,
@@ -640,16 +641,17 @@
Text.append(UseCRLF ? "\r\n" : "\n");
}
-void WhitespaceManager::appendNewlineText(std::string , unsigned Newlines,
- unsigned PreviousEndOfTokenColumn,
- unsigned EscapedNewlineColumn) {
+void WhitespaceManager::appendEscapedNewlineText(std::string ,
+ unsigned Newlines,
+ unsigned
PreviousEndOfTokenColumn,
+ unsigned
EscapedNewlineColumn) {
if (Newlines > 0) {
-unsigned Offset =
-std::min(EscapedNewlineColumn - 2, PreviousEndOfTokenColumn);
+unsigned Spaces =
+std::max(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(0, EscapedNewlineColumn - 1);
}
}
}
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -2309,6 +2309,30 @@
EXPECT_EQ("template f();", format("\\\ntemplate f();"));
EXPECT_EQ("/* \\ \\ \\\n */", format("\\\n/* \\ \\ \\\n */"));
EXPECT_EQ("", format(""));
+
+ 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) {
Index: cfe/trunk/lib/Format/WhitespaceManager.h
===
--- cfe/trunk/lib/Format/WhitespaceManager.h
+++ cfe/trunk/lib/Format/WhitespaceManager.h
@@ -195,9 +195,9 @@
/// \brief Stores \p Text as the replacement for the whitespace in \p Range.
void storeReplacement(SourceRange Range, StringRef Text);
void appendNewlineText(std::string , unsigned
[PATCH] D36019: [clang-format] Fix bug with ENAS_DontAlign and empty lines
jtbandes added a comment. Thanks. Can you commit this when you get a chance? I don't have permissions. https://reviews.llvm.org/D36019 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36019: [clang-format] Fix bug with ENAS_DontAlign and empty lines
djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Thanks you. Comment at: lib/Format/WhitespaceManager.cpp:650 +for (unsigned i = 0; i < Newlines; ++i) + Text.append(UseCRLF ? " \\\r\n" : " \\\n"); +return; jtbandes wrote: > djasper wrote: > > 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. > I suppose that can be changed back to 1 by using `std::max(1, > EscapedNewlineColumn - 1);` instead, right? I don't have strong feelings > about whether it should be 0 or 1. I don't either. Leave as is for now. https://reviews.llvm.org/D36019 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36019: [clang-format] Fix bug with ENAS_DontAlign and empty lines
jtbandes added inline comments. Comment at: lib/Format/WhitespaceManager.cpp:650 +for (unsigned i = 0; i < Newlines; ++i) + Text.append(UseCRLF ? " \\\r\n" : " \\\n"); +return; djasper wrote: > 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. I suppose that can be changed back to 1 by using `std::max(1, EscapedNewlineColumn - 1);` instead, right? I don't have strong feelings about whether it should be 0 or 1. https://reviews.llvm.org/D36019 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36019: [clang-format] Fix bug with ENAS_DontAlign and empty lines
jtbandes updated this revision to Diff 109898.
jtbandes added a comment.
@djasper ok, done
https://reviews.llvm.org/D36019
Files:
lib/Format/WhitespaceManager.cpp
lib/Format/WhitespaceManager.h
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2309,6 +2309,30 @@
EXPECT_EQ("template f();", format("\\\ntemplate f();"));
EXPECT_EQ("/* \\ \\ \\\n */", format("\\\n/* \\ \\ \\\n */"));
EXPECT_EQ("", format(""));
+
+ 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) {
Index: lib/Format/WhitespaceManager.h
===
--- lib/Format/WhitespaceManager.h
+++ lib/Format/WhitespaceManager.h
@@ -195,9 +195,9 @@
/// \brief Stores \p Text as the replacement for the whitespace in \p Range.
void storeReplacement(SourceRange Range, StringRef Text);
void appendNewlineText(std::string , unsigned Newlines);
- void appendNewlineText(std::string , unsigned Newlines,
- unsigned PreviousEndOfTokenColumn,
- unsigned EscapedNewlineColumn);
+ void appendEscapedNewlineText(std::string , unsigned Newlines,
+unsigned PreviousEndOfTokenColumn,
+unsigned EscapedNewlineColumn);
void appendIndentText(std::string , unsigned IndentLevel,
unsigned Spaces, unsigned WhitespaceStartColumn);
Index: lib/Format/WhitespaceManager.cpp
===
--- lib/Format/WhitespaceManager.cpp
+++ lib/Format/WhitespaceManager.cpp
@@ -603,8 +603,9 @@
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,
@@ -640,16 +641,17 @@
Text.append(UseCRLF ? "\r\n" : "\n");
}
-void WhitespaceManager::appendNewlineText(std::string , unsigned Newlines,
- unsigned PreviousEndOfTokenColumn,
- unsigned EscapedNewlineColumn) {
+void WhitespaceManager::appendEscapedNewlineText(std::string ,
+ unsigned Newlines,
+ unsigned
PreviousEndOfTokenColumn,
+ unsigned
EscapedNewlineColumn) {
if (Newlines > 0) {
-unsigned Offset =
-std::min(EscapedNewlineColumn - 2, PreviousEndOfTokenColumn);
+unsigned Spaces =
+std::max(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(0, EscapedNewlineColumn - 1);
}
}
}
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2309,6 +2309,30 @@
EXPECT_EQ("template f();", format("\\\ntemplate f();"));
EXPECT_EQ("/* \\ \\ \\\n */", format("\\\n/* \\ \\ \\\n */"));
EXPECT_EQ("", format(""));
+
+ 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
[PATCH] D36019: [clang-format] Fix bug with ENAS_DontAlign and empty lines
djasper added inline comments.
Comment at: lib/Format/WhitespaceManager.cpp:650
+for (unsigned i = 0; i < Newlines; ++i)
+ Text.append(UseCRLF ? " \\\r\n" : " \\\n");
+return;
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.
Comment at: lib/Format/WhitespaceManager.cpp:656
+assert(EscapedNewlineColumn >= 1);
unsigned Offset =
+std::min(EscapedNewlineColumn - 1, PreviousEndOfTokenColumn);
You could change this to:
unsigned Spaces =
std::max(1, EscapedNewlineColumn - PreviousEndOfTokenColumn - 1);
for (unsigned i = 0; i < Newlines; ++i) {
Text.append(Spaces, ' ');
Text.append(UseCRLF ? "\\\r\n" : "\\\n");
Spaces = std::max(0, EscapedNewlineColumn - 1);
}
And it should work without problems and without special code path.
https://reviews.llvm.org/D36019
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36019: [clang-format] Fix bug with ENAS_DontAlign and empty lines
jtbandes added a comment. @djasper Bump :) https://reviews.llvm.org/D36019 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36019: [clang-format] Fix bug with ENAS_DontAlign and empty lines
jtbandes updated this revision to Diff 108863.
jtbandes added a comment.
- Undo change in argument list
https://reviews.llvm.org/D36019
Files:
lib/Format/WhitespaceManager.cpp
lib/Format/WhitespaceManager.h
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2309,6 +2309,30 @@
EXPECT_EQ("template f();", format("\\\ntemplate f();"));
EXPECT_EQ("/* \\ \\ \\\n */", format("\\\n/* \\ \\ \\\n */"));
EXPECT_EQ("", format(""));
+
+ 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) {
Index: lib/Format/WhitespaceManager.h
===
--- lib/Format/WhitespaceManager.h
+++ lib/Format/WhitespaceManager.h
@@ -195,9 +195,9 @@
/// \brief Stores \p Text as the replacement for the whitespace in \p Range.
void storeReplacement(SourceRange Range, StringRef Text);
void appendNewlineText(std::string , unsigned Newlines);
- void appendNewlineText(std::string , unsigned Newlines,
- unsigned PreviousEndOfTokenColumn,
- unsigned EscapedNewlineColumn);
+ void appendEscapedNewlineText(std::string , unsigned Newlines,
+unsigned PreviousEndOfTokenColumn,
+unsigned EscapedNewlineColumn);
void appendIndentText(std::string , unsigned IndentLevel,
unsigned Spaces, unsigned WhitespaceStartColumn);
Index: lib/Format/WhitespaceManager.cpp
===
--- lib/Format/WhitespaceManager.cpp
+++ lib/Format/WhitespaceManager.cpp
@@ -603,8 +603,9 @@
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,
@@ -640,12 +641,20 @@
Text.append(UseCRLF ? "\r\n" : "\n");
}
-void WhitespaceManager::appendNewlineText(std::string , unsigned Newlines,
- unsigned PreviousEndOfTokenColumn,
- unsigned EscapedNewlineColumn) {
+void WhitespaceManager::appendEscapedNewlineText(std::string ,
+ unsigned Newlines,
+ unsigned
PreviousEndOfTokenColumn,
+ unsigned
EscapedNewlineColumn) {
+ if (Style.AlignEscapedNewlines == FormatStyle::ENAS_DontAlign) {
+for (unsigned i = 0; i < Newlines; ++i)
+ Text.append(UseCRLF ? " \\\r\n" : " \\\n");
+return;
+ }
+
if (Newlines > 0) {
+assert(EscapedNewlineColumn >= 1);
unsigned Offset =
-std::min(EscapedNewlineColumn - 2, PreviousEndOfTokenColumn);
+std::min(EscapedNewlineColumn - 1, PreviousEndOfTokenColumn);
for (unsigned i = 0; i < Newlines; ++i) {
Text.append(EscapedNewlineColumn - Offset - 1, ' ');
Text.append(UseCRLF ? "\\\r\n" : "\\\n");
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2309,6 +2309,30 @@
EXPECT_EQ("template f();", format("\\\ntemplate f();"));
EXPECT_EQ("/* \\ \\ \\\n */", format("\\\n/* \\ \\ \\\n */"));
EXPECT_EQ("", format(""));
+
+ FormatStyle DontAlign = getLLVMStyle();
+ DontAlign.AlignEscapedNewlines = FormatStyle::ENAS_DontAlign;
+ DontAlign.MaxEmptyLinesToKeep = 3;
+ // FIXME: can't
[PATCH] D36019: [clang-format] Fix bug with ENAS_DontAlign and empty lines
jtbandes updated this revision to Diff 108860.
jtbandes added a comment.
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 https://reviews.llvm.org/D32733. I don't think there was a good
reason to change this.
- Leave in the `assert`.
https://reviews.llvm.org/D36019
Files:
lib/Format/WhitespaceManager.cpp
lib/Format/WhitespaceManager.h
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2309,6 +2309,30 @@
EXPECT_EQ("template f();", format("\\\ntemplate f();"));
EXPECT_EQ("/* \\ \\ \\\n */", format("\\\n/* \\ \\ \\\n */"));
EXPECT_EQ("", format(""));
+
+ 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) {
Index: lib/Format/WhitespaceManager.h
===
--- lib/Format/WhitespaceManager.h
+++ lib/Format/WhitespaceManager.h
@@ -195,9 +195,7 @@
/// \brief Stores \p Text as the replacement for the whitespace in \p Range.
void storeReplacement(SourceRange Range, StringRef Text);
void appendNewlineText(std::string , unsigned Newlines);
- void appendNewlineText(std::string , unsigned Newlines,
- unsigned PreviousEndOfTokenColumn,
- unsigned EscapedNewlineColumn);
+ void appendEscapedNewlineText(std::string , const Change );
void appendIndentText(std::string , unsigned IndentLevel,
unsigned Spaces, unsigned WhitespaceStartColumn);
Index: lib/Format/WhitespaceManager.cpp
===
--- lib/Format/WhitespaceManager.cpp
+++ lib/Format/WhitespaceManager.cpp
@@ -603,8 +603,7 @@
if (C.CreateReplacement) {
std::string ReplacementText = C.PreviousLinePostfix;
if (C.ContinuesPPDirective)
-appendNewlineText(ReplacementText, C.NewlinesBefore,
- C.PreviousEndOfTokenColumn, C.EscapedNewlineColumn);
+appendEscapedNewlineText(ReplacementText, C);
else
appendNewlineText(ReplacementText, C.NewlinesBefore);
appendIndentText(ReplacementText, C.Tok->IndentLevel,
@@ -640,14 +639,19 @@
Text.append(UseCRLF ? "\r\n" : "\n");
}
-void WhitespaceManager::appendNewlineText(std::string , unsigned Newlines,
- unsigned PreviousEndOfTokenColumn,
- unsigned EscapedNewlineColumn) {
- if (Newlines > 0) {
+void WhitespaceManager::appendEscapedNewlineText(std::string , const
Change ) {
+ if (Style.AlignEscapedNewlines == FormatStyle::ENAS_DontAlign) {
+for (unsigned i = 0; i < C.NewlinesBefore; ++i)
+ Text.append(UseCRLF ? " \\\r\n" : " \\\n");
+return;
+ }
+
+ if (C.NewlinesBefore > 0) {
+assert(C.EscapedNewlineColumn >= 1);
unsigned Offset =
-std::min(EscapedNewlineColumn - 2, PreviousEndOfTokenColumn);
-for (unsigned i = 0; i < Newlines; ++i) {
- Text.append(EscapedNewlineColumn - Offset - 1, ' ');
+std::min(C.EscapedNewlineColumn - 1, C.PreviousEndOfTokenColumn);
+for (unsigned i = 0; i < C.NewlinesBefore; ++i) {
+ Text.append(C.EscapedNewlineColumn - Offset - 1, ' ');
Text.append(UseCRLF ? "\\\r\n" : "\\\n");
Offset = 0;
}
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2309,6 +2309,30 @@
EXPECT_EQ("template f();", format("\\\ntemplate f();"));
EXPECT_EQ("/* \\ \\ \\\n */", format("\\\n/* \\ \\ \\\n */"));
EXPECT_EQ("", format(""));
+
+ FormatStyle DontAlign =
[PATCH] D36019: [clang-format] Fix bug with ENAS_DontAlign and empty lines
djasper added a comment. 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? https://reviews.llvm.org/D36019 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36019: [clang-format] Fix bug with ENAS_DontAlign and empty lines
jtbandes created this revision.
Herald added a subscriber: klimek.
This fixes a bug in `ENAS_DontAlign` (introduced in
https://reviews.llvm.org/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=97404, now with a proper
justification :)
https://reviews.llvm.org/D36019
Files:
lib/Format/WhitespaceManager.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2309,6 +2309,30 @@
EXPECT_EQ("template f();", format("\\\ntemplate f();"));
EXPECT_EQ("/* \\ \\ \\\n */", format("\\\n/* \\ \\ \\\n */"));
EXPECT_EQ("", format(""));
+
+ 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) {
Index: lib/Format/WhitespaceManager.cpp
===
--- lib/Format/WhitespaceManager.cpp
+++ lib/Format/WhitespaceManager.cpp
@@ -557,9 +557,22 @@
}
void WhitespaceManager::alignEscapedNewlines() {
- if (Style.AlignEscapedNewlines == FormatStyle::ENAS_DontAlign)
+ // If we are not aligning escaped newlines, just set EscapedNewlineColumn
+ // to point to the end of each line.
+ if (Style.AlignEscapedNewlines == FormatStyle::ENAS_DontAlign) {
+bool PreviousContinuerHadNLBefore = false; // used to detect blank lines
+for (Change : Changes) {
+ if (C.ContinuesPPDirective) {
+if (C.NewlinesBefore > 0)
+ C.EscapedNewlineColumn =
+PreviousContinuerHadNLBefore ? 2 : C.PreviousEndOfTokenColumn + 2;
+PreviousContinuerHadNLBefore = C.NewlinesBefore > 0;
+ }
+}
return;
+ }
+ // Otherwise, compute the max width and then apply it to all lines.
bool AlignLeft = Style.AlignEscapedNewlines == FormatStyle::ENAS_Left;
unsigned MaxEndOfLine = AlignLeft ? 0 : Style.ColumnLimit;
unsigned StartOfMacro = 0;
@@ -644,6 +657,7 @@
unsigned PreviousEndOfTokenColumn,
unsigned EscapedNewlineColumn) {
if (Newlines > 0) {
+assert(EscapedNewlineColumn >= 2);
unsigned Offset =
std::min(EscapedNewlineColumn - 2, PreviousEndOfTokenColumn);
for (unsigned i = 0; i < Newlines; ++i) {
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2309,6 +2309,30 @@
EXPECT_EQ("template f();", format("\\\ntemplate f();"));
EXPECT_EQ("/* \\ \\ \\\n */", format("\\\n/* \\ \\ \\\n */"));
EXPECT_EQ("", format(""));
+
+ 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) {
Index: lib/Format/WhitespaceManager.cpp
===
--- lib/Format/WhitespaceManager.cpp
+++ lib/Format/WhitespaceManager.cpp
@@ -557,9 +557,22 @@
}
void WhitespaceManager::alignEscapedNewlines() {
