[PATCH] D125137: [clang-format] Don't remove braces if a 1-statement body would wrap
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb6d8c84f2810: [clang-format] Dont remove braces if a
1-statement body would wrap (authored by owenpan).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125137/new/
https://reviews.llvm.org/D125137
Files:
clang/lib/Format/UnwrappedLineParser.cpp
clang/lib/Format/UnwrappedLineParser.h
clang/unittests/Format/FormatTest.cpp
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -25365,8 +25365,6 @@
"}",
Style);
- // FIXME: See https://github.com/llvm/llvm-project/issues/53543.
-#if 0
Style.ColumnLimit = 65;
verifyFormat("if (condition) {\n"
@@ -25380,6 +25378,15 @@
Style.ColumnLimit = 20;
+ verifyFormat("int ab = [](int i) {\n"
+ " if (i > 0) {\n"
+ "i = 12345678 -\n"
+ "i;\n"
+ " }\n"
+ " return i;\n"
+ "};",
+ Style);
+
verifyFormat("if (a) {\n"
" b = c + // 1 -\n"
" d;\n"
@@ -25394,9 +25401,6 @@
" b = c >= 0 ? d : e;\n"
"}",
Style);
-#endif
-
- Style.ColumnLimit = 20;
verifyFormat("if (a)\n"
" b = c > 0 ? d : e;",
Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -95,6 +95,7 @@
bool parseLevel(bool HasOpeningBrace, bool CanContainBracedList,
IfStmtKind *IfKind = nullptr,
TokenType NextLBracesType = TT_Unknown);
+ bool mightFitOnOneLine(UnwrappedLine ) const;
IfStmtKind parseBlock(bool MustBeDeclaration = false, unsigned AddLevels = 1u,
bool MunchSemi = true,
bool UnindentWhitesmithsBraces = false,
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -14,6 +14,7 @@
#include "UnwrappedLineParser.h"
#include "FormatToken.h"
+#include "TokenAnnotator.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/Support/Debug.h"
#include "llvm/Support/raw_ostream.h"
@@ -460,6 +461,7 @@
return Previous && Previous->is(tok::comment) &&
(Previous->IsMultiline || Previous->NewlinesBefore > 0);
}
+
/// \brief Parses a level, that is ???.
/// \param HasOpeningBrace If that level is started by an opening brace.
/// \param CanContainBracedList If the content can contain (at any level) a
@@ -751,6 +753,50 @@
return h;
}
+// Checks whether \p ParsedLine might fit on a single line. We must clone the
+// tokens of \p ParsedLine before running the token annotator on it so that we
+// can restore them afterward.
+bool UnwrappedLineParser::mightFitOnOneLine(UnwrappedLine ) const {
+ const auto ColumnLimit = Style.ColumnLimit;
+ if (ColumnLimit == 0)
+return true;
+
+ auto = ParsedLine.Tokens;
+ assert(!Tokens.empty());
+ const auto *LastToken = Tokens.back().Tok;
+ assert(LastToken);
+
+ SmallVector SavedTokens(Tokens.size());
+
+ int Index = 0;
+ for (const auto : Tokens) {
+assert(Token.Tok);
+auto = SavedTokens[Index++];
+SavedToken.Tok = new FormatToken;
+SavedToken.Tok->copyFrom(*Token.Tok);
+SavedToken.Children = std::move(Token.Children);
+ }
+
+ AnnotatedLine Line(ParsedLine);
+ assert(Line.Last == LastToken);
+
+ TokenAnnotator Annotator(Style, Keywords);
+ Annotator.annotate(Line);
+ Annotator.calculateFormattingInformation(Line);
+
+ const int Length = LastToken->TotalLength;
+
+ Index = 0;
+ for (auto : Tokens) {
+const auto = SavedTokens[Index++];
+Token.Tok->copyFrom(*SavedToken.Tok);
+Token.Children = std::move(SavedToken.Children);
+delete SavedToken.Tok;
+ }
+
+ return Line.Level * Style.IndentWidth + Length <= ColumnLimit;
+}
+
UnwrappedLineParser::IfStmtKind
UnwrappedLineParser::parseBlock(bool MustBeDeclaration, unsigned AddLevels,
bool MunchSemi, bool UnindentWhitesmithsBraces,
@@ -813,8 +859,11 @@
const FormatToken *Previous = Tokens->getPreviousToken();
assert(Previous);
if (Previous->isNot(tok::r_brace) || Previous->Optional) {
- Tok->MatchingParen = FormatTok;
- FormatTok->MatchingParen = Tok;
+ assert(!CurrentLines->empty());
+ if (mightFitOnOneLine(CurrentLines->back())) {
+Tok->MatchingParen = FormatTok;
+FormatTok->MatchingParen = Tok;
+ }
}
}
___
cfe-commits
[PATCH] D125137: [clang-format] Don't remove braces if a 1-statement body would wrap
curdeius added inline comments. Comment at: clang/lib/Format/UnwrappedLineParser.cpp:787 + + const int Length = LastToken->TotalLength; + owenpan wrote: > curdeius wrote: > > Why not like this? > > Why not like this? > > See the assertion on line 781 above. We are computing the `TotalLength` of > `LastToken` via `Line`. Either way works, but I prefer the simpler > expression. I can change it though if you insist. Yeah, what I meant is to just get rid of `LastToken` variable and write the suggested code. But well, both ways work. No strong feelings. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125137/new/ https://reviews.llvm.org/D125137 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D125137: [clang-format] Don't remove braces if a 1-statement body would wrap
owenpan added inline comments. Comment at: clang/lib/Format/UnwrappedLineParser.cpp:766 + assert(!Tokens.empty()); + const auto *LastToken = Tokens.back().Tok; + assert(LastToken); curdeius wrote: > It might be a matter of taste but adding this variable makes the code harder > to read to me. The last token of `ParsedLine` is of interest here. We want the annotator to compute its `TotalLength` to determine whether the line might fit on a single line. I'm open to renaming the variable if you have a better suggestion. Comment at: clang/lib/Format/UnwrappedLineParser.cpp:777 +SavedToken.Tok->copyFrom(*Token.Tok); +SavedToken.Children = std::move(Token.Children); + } curdeius wrote: > So the token's children are modified (moved)? Is it done so that children be > not considered by the annotator? Both the constructor and the destructor of `AnnotatedLine` clear the children of `UnwrappedLineNode`, so we save them beforehand and restore them afterward. Comment at: clang/lib/Format/UnwrappedLineParser.cpp:787 + + const int Length = LastToken->TotalLength; + curdeius wrote: > Why not like this? > Why not like this? See the assertion on line 781 above. We are computing the `TotalLength` of `LastToken` via `Line`. Either way works, but I prefer the simpler expression. I can change it though if you insist. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125137/new/ https://reviews.llvm.org/D125137 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D125137: [clang-format] Don't remove braces if a 1-statement body would wrap
curdeius accepted this revision. curdeius added a comment. This revision is now accepted and ready to land. LGTM with nits. Comment at: clang/lib/Format/UnwrappedLineParser.cpp:766 + assert(!Tokens.empty()); + const auto *LastToken = Tokens.back().Tok; + assert(LastToken); It might be a matter of taste but adding this variable makes the code harder to read to me. Comment at: clang/lib/Format/UnwrappedLineParser.cpp:777 +SavedToken.Tok->copyFrom(*Token.Tok); +SavedToken.Children = std::move(Token.Children); + } So the token's children are modified (moved)? Is it done so that children be not considered by the annotator? Comment at: clang/lib/Format/UnwrappedLineParser.cpp:787 + + const int Length = LastToken->TotalLength; + Why not like this? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125137/new/ https://reviews.llvm.org/D125137 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D125137: [clang-format] Don't remove braces if a 1-statement body would wrap
owenpan updated this revision to Diff 428468.
owenpan added a comment.
Added saving/restoring the children of `UnwrappedLineNode` and addressed review
comments.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125137/new/
https://reviews.llvm.org/D125137
Files:
clang/lib/Format/UnwrappedLineParser.cpp
clang/lib/Format/UnwrappedLineParser.h
clang/unittests/Format/FormatTest.cpp
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -25351,8 +25351,6 @@
"}",
Style);
- // FIXME: See https://github.com/llvm/llvm-project/issues/53543.
-#if 0
Style.ColumnLimit = 65;
verifyFormat("if (condition) {\n"
@@ -25366,6 +25364,15 @@
Style.ColumnLimit = 20;
+ verifyFormat("int ab = [](int i) {\n"
+ " if (i > 0) {\n"
+ "i = 12345678 -\n"
+ "i;\n"
+ " }\n"
+ " return i;\n"
+ "};",
+ Style);
+
verifyFormat("if (a) {\n"
" b = c + // 1 -\n"
" d;\n"
@@ -25380,9 +25387,6 @@
" b = c >= 0 ? d : e;\n"
"}",
Style);
-#endif
-
- Style.ColumnLimit = 20;
verifyFormat("if (a)\n"
" b = c > 0 ? d : e;",
Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -95,6 +95,7 @@
bool parseLevel(bool HasOpeningBrace, bool CanContainBracedList,
IfStmtKind *IfKind = nullptr,
TokenType NextLBracesType = TT_Unknown);
+ bool mightFitOnOneLine(UnwrappedLine ) const;
IfStmtKind parseBlock(bool MustBeDeclaration = false, unsigned AddLevels = 1u,
bool MunchSemi = true,
bool UnindentWhitesmithsBraces = false,
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -14,6 +14,7 @@
#include "UnwrappedLineParser.h"
#include "FormatToken.h"
+#include "TokenAnnotator.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/Support/Debug.h"
#include "llvm/Support/raw_ostream.h"
@@ -460,6 +461,7 @@
return Previous && Previous->is(tok::comment) &&
(Previous->IsMultiline || Previous->NewlinesBefore > 0);
}
+
/// \brief Parses a level, that is ???.
/// \param HasOpeningBrace If that level is started by an opening brace.
/// \param CanContainBracedList If the content can contain (at any level) a
@@ -751,6 +753,50 @@
return h;
}
+// Checks whether \p ParsedLine might fit on a single line. We must clone the
+// tokens of \p ParsedLine before running the token annotator on it so that we
+// can restore them afterward.
+bool UnwrappedLineParser::mightFitOnOneLine(UnwrappedLine ) const {
+ const auto ColumnLimit = Style.ColumnLimit;
+ if (ColumnLimit == 0)
+return true;
+
+ auto = ParsedLine.Tokens;
+ assert(!Tokens.empty());
+ const auto *LastToken = Tokens.back().Tok;
+ assert(LastToken);
+
+ SmallVector SavedTokens(Tokens.size());
+
+ int Index = 0;
+ for (const auto : Tokens) {
+assert(Token.Tok);
+auto = SavedTokens[Index++];
+SavedToken.Tok = new FormatToken;
+SavedToken.Tok->copyFrom(*Token.Tok);
+SavedToken.Children = std::move(Token.Children);
+ }
+
+ AnnotatedLine Line(ParsedLine);
+ assert(Line.Last == LastToken);
+
+ TokenAnnotator Annotator(Style, Keywords);
+ Annotator.annotate(Line);
+ Annotator.calculateFormattingInformation(Line);
+
+ const int Length = LastToken->TotalLength;
+
+ Index = 0;
+ for (auto : Tokens) {
+const auto = SavedTokens[Index++];
+Token.Tok->copyFrom(*SavedToken.Tok);
+Token.Children = std::move(SavedToken.Children);
+delete SavedToken.Tok;
+ }
+
+ return Line.Level * Style.IndentWidth + Length <= ColumnLimit;
+}
+
UnwrappedLineParser::IfStmtKind
UnwrappedLineParser::parseBlock(bool MustBeDeclaration, unsigned AddLevels,
bool MunchSemi, bool UnindentWhitesmithsBraces,
@@ -813,8 +859,11 @@
const FormatToken *Previous = Tokens->getPreviousToken();
assert(Previous);
if (Previous->isNot(tok::r_brace) || Previous->Optional) {
- Tok->MatchingParen = FormatTok;
- FormatTok->MatchingParen = Tok;
+ assert(!CurrentLines->empty());
+ if (mightFitOnOneLine(CurrentLines->back())) {
+Tok->MatchingParen = FormatTok;
+FormatTok->MatchingParen = Tok;
+ }
}
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D125137: [clang-format] Don't remove braces if a 1-statement body would wrap
owenpan planned changes to this revision.
owenpan added inline comments.
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:465
+
+bool UnwrappedLineParser::mightFitOnOneLine() const {
+ const auto ColumnLimit = Style.ColumnLimit;
HazardyKnusperkeks wrote:
> A bit explanation what it means that something //might// fit in one line
> would be nice.
Will do.
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:479
+ if (!LastToken->isOneOf(tok::semi, tok::comment))
+return true;
+
HazardyKnusperkeks wrote:
> Especially this one is not clear to me, why do we return true here?
We will remove braces only if the previous line (i.e. the line before the
closing brace) ends with a semi or comment. I should move this check to the
caller or rename the function.
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:481
+
+ SmallVector SavedTokens;
+ for (const auto : PreviousLine.Tokens) {
HazardyKnusperkeks wrote:
> Is a FormatToken "big", or expensive to copy? If not I'd save them directly,
> otherwise I'd prefer a unique_ptr.
We have to make a copy of the FormatToken here so that we can restore it after
the TokenAnnotator modifies it directly.
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:484
+FormatToken *Tok = new FormatToken;
+Tok->copyFrom(*Token.Tok);
+SavedTokens.push_back(Tok);
We must save `Token.Children` as well.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125137/new/
https://reviews.llvm.org/D125137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D125137: [clang-format] Don't remove braces if a 1-statement body would wrap
HazardyKnusperkeks added inline comments.
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:465
+
+bool UnwrappedLineParser::mightFitOnOneLine() const {
+ const auto ColumnLimit = Style.ColumnLimit;
A bit explanation what it means that something //might// fit in one line would
be nice.
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:479
+ if (!LastToken->isOneOf(tok::semi, tok::comment))
+return true;
+
Especially this one is not clear to me, why do we return true here?
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:481
+
+ SmallVector SavedTokens;
+ for (const auto : PreviousLine.Tokens) {
Is a FormatToken "big", or expensive to copy? If not I'd save them directly,
otherwise I'd prefer a unique_ptr.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125137/new/
https://reviews.llvm.org/D125137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D125137: [clang-format] Don't remove braces if a 1-statement body would wrap
owenpan added inline comments.
Comment at: clang/unittests/Format/FormatTest.cpp:25374
verifyFormat("if (a) {\n"
" b = c >= 0 ? d\n"
" : e;\n"
Jeroen wrote:
> Will it also add braces if they where not there yet?
No, but you can insert braces by setting `InsertBraces` to true and
`RemoveBracesLLVM` to false.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125137/new/
https://reviews.llvm.org/D125137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D125137: [clang-format] Don't remove braces if a 1-statement body would wrap
Jeroen added a comment.
Will it also add braces if they where not there yet?
Comment at: clang/unittests/Format/FormatTest.cpp:25374
verifyFormat("if (a) {\n"
" b = c >= 0 ? d\n"
" : e;\n"
Will it also add braces if they where not there yet?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125137/new/
https://reviews.llvm.org/D125137
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D125137: [clang-format] Don't remove braces if a 1-statement body would wrap
owenpan created this revision.
owenpan added reviewers: curdeius, HazardyKnusperkeks, MyDeveloperDay.
owenpan added a project: clang-format.
Herald added a project: All.
owenpan requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Reimplement the `RemoveBracesLLVM` feature which handles a single-statement
block that would get wrapped.
Fixes https://github.com/llvm/llvm-project/issues/53543.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D125137
Files:
clang/lib/Format/UnwrappedLineParser.cpp
clang/lib/Format/UnwrappedLineParser.h
clang/unittests/Format/FormatTest.cpp
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -25351,8 +25351,6 @@
"}",
Style);
- // FIXME: See https://github.com/llvm/llvm-project/issues/53543.
-#if 0
Style.ColumnLimit = 65;
verifyFormat("if (condition) {\n"
@@ -25380,9 +25378,6 @@
" b = c >= 0 ? d : e;\n"
"}",
Style);
-#endif
-
- Style.ColumnLimit = 20;
verifyFormat("if (a)\n"
" b = c > 0 ? d : e;",
Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -92,6 +92,7 @@
void reset();
void parseFile();
bool precededByCommentOrPPDirective() const;
+ bool mightFitOnOneLine() const;
bool parseLevel(bool HasOpeningBrace, bool CanContainBracedList,
IfStmtKind *IfKind = nullptr,
TokenType NextLBracesType = TT_Unknown);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -14,6 +14,7 @@
#include "UnwrappedLineParser.h"
#include "FormatToken.h"
+#include "TokenAnnotator.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/Support/Debug.h"
#include "llvm/Support/raw_ostream.h"
@@ -460,13 +461,56 @@
return Previous && Previous->is(tok::comment) &&
(Previous->IsMultiline || Previous->NewlinesBefore > 0);
}
+
+bool UnwrappedLineParser::mightFitOnOneLine() const {
+ const auto ColumnLimit = Style.ColumnLimit;
+ if (ColumnLimit == 0)
+return true;
+
+ if (Lines.empty())
+return true;
+
+ const auto = Lines.back();
+ const auto = PreviousLine.Tokens;
+ assert(!Tokens.empty());
+ const auto *LastToken = Tokens.back().Tok;
+ assert(LastToken);
+ if (!LastToken->isOneOf(tok::semi, tok::comment))
+return true;
+
+ SmallVector SavedTokens;
+ for (const auto : PreviousLine.Tokens) {
+FormatToken *Tok = new FormatToken;
+Tok->copyFrom(*Token.Tok);
+SavedTokens.push_back(Tok);
+ }
+
+ AnnotatedLine Line(PreviousLine);
+ assert(Line.Last == LastToken);
+
+ TokenAnnotator Annotator(Style, Keywords);
+ Annotator.annotate(Line);
+ Annotator.calculateFormattingInformation(Line);
+
+ const int Length = LastToken->TotalLength;
+
+ int I = 0;
+ for (const auto : PreviousLine.Tokens) {
+const FormatToken *Tok = SavedTokens[I++];
+Token.Tok->copyFrom(*Tok);
+delete Tok;
+ }
+
+ return Line.Level * Style.IndentWidth + Length <= ColumnLimit;
+}
+
/// \brief Parses a level, that is ???.
/// \param HasOpeningBrace If that level is started by an opening brace.
/// \param CanContainBracedList If the content can contain (at any level) a
/// braced list.
/// \param NextLBracesType The type for left brace found in this level.
/// \returns true if a simple block, or false otherwise. (A simple block has a
-/// single statement.)
+/// single statement that fits on a single line.)
bool UnwrappedLineParser::parseLevel(bool HasOpeningBrace,
bool CanContainBracedList,
IfStmtKind *IfKind,
@@ -533,7 +577,9 @@
precededByCommentOrPPDirective())
return false;
const FormatToken *Next = Tokens->peekNextToken();
-return Next->isNot(tok::comment) || Next->NewlinesBefore > 0;
+if (Next->is(tok::comment) && Next->NewlinesBefore == 0)
+ return false;
+return mightFitOnOneLine();
}
nextToken();
addUnwrappedLine();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
