[PATCH] D39900: Refactor ContinuationIndenter's breakProtrudingToken logic into slightly moreorthogonal pieces.

2017-11-14 Thread Manuel Klimek via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL318141: Refactor ContinuationIndenter's breakProtrudingToken 
logic. (authored by klimek).

Repository:
  rL LLVM

https://reviews.llvm.org/D39900

Files:
  cfe/trunk/lib/Format/BreakableToken.cpp
  cfe/trunk/lib/Format/ContinuationIndenter.cpp
  cfe/trunk/lib/Format/ContinuationIndenter.h
  cfe/trunk/unittests/Format/FormatTest.cpp
  cfe/trunk/unittests/Format/FormatTestComments.cpp

Index: cfe/trunk/lib/Format/BreakableToken.cpp
===
--- cfe/trunk/lib/Format/BreakableToken.cpp
+++ cfe/trunk/lib/Format/BreakableToken.cpp
@@ -749,6 +749,7 @@
 Prefix.resize(Lines.size());
 OriginalPrefix.resize(Lines.size());
 for (size_t i = FirstLineIndex, e = Lines.size(); i < e; ++i) {
+  Lines[i] = Lines[i].ltrim(Blanks);
   // We need to trim the blanks in case this is not the first line in a
   // multiline comment. Then the indent is included in Lines[i].
   StringRef IndentPrefix =
Index: cfe/trunk/lib/Format/ContinuationIndenter.h
===
--- cfe/trunk/lib/Format/ContinuationIndenter.h
+++ cfe/trunk/lib/Format/ContinuationIndenter.h
@@ -29,6 +29,7 @@
 namespace format {
 
 class AnnotatedLine;
+class BreakableToken;
 struct FormatToken;
 struct LineState;
 struct ParenState;
@@ -105,11 +106,20 @@
   /// 
   /// \returns An extra penalty induced by reformatting the token.
   unsigned reformatRawStringLiteral(const FormatToken ,
-unsigned StartColumn, LineState ,
-StringRef Delimiter,
+LineState ,
 const FormatStyle ,
 bool DryRun);
 
+  /// \brief If the current token is at the end of the current line, handle
+  /// the transition to the next line.
+  unsigned handleEndOfLine(const FormatToken , LineState ,
+   bool DryRun, bool AllowBreak);
+
+  /// \brief If \p Current is a raw string that is configured to be reformatted,
+  /// return the style to be used.
+  llvm::Optional getRawStringStyle(const FormatToken ,
+const LineState );
+
   /// \brief If the current token sticks out over the end of the line, break
   /// it if possible.
   ///
@@ -120,7 +130,13 @@
   /// penalty for the column limit violation in the last line (and in single
   /// line tokens) is handled in \c addNextStateToQueue.
   unsigned breakProtrudingToken(const FormatToken , LineState ,
-bool DryRun);
+bool AllowBreak, bool DryRun);
+
+  /// \brief Returns the \c BreakableToken starting at \p Current, or nullptr
+  /// if the current token cannot be broken.
+  std::unique_ptr
+  createBreakableToken(const FormatToken , LineState ,
+   bool AllowBreak);
 
   /// \brief Appends the next token to \p State and updates information
   /// necessary for indentation.
Index: cfe/trunk/lib/Format/ContinuationIndenter.cpp
===
--- cfe/trunk/lib/Format/ContinuationIndenter.cpp
+++ cfe/trunk/lib/Format/ContinuationIndenter.cpp
@@ -1008,8 +1008,8 @@
 
   moveStatePastFakeLParens(State, Newline);
   moveStatePastScopeCloser(State);
-  bool CanBreakProtrudingToken = !State.Stack.back().NoLineBreak &&
- !State.Stack.back().NoLineBreakInOperand;
+  bool AllowBreak = !State.Stack.back().NoLineBreak &&
+!State.Stack.back().NoLineBreakInOperand;
   moveStatePastScopeOpener(State, Newline);
   moveStatePastFakeRParens(State);
 
@@ -1023,13 +1023,9 @@
 
   State.Column += Current.ColumnWidth;
   State.NextToken = State.NextToken->Next;
-  unsigned Penalty = 0;
-  if (CanBreakProtrudingToken)
-Penalty = breakProtrudingToken(Current, State, DryRun);
-  if (State.Column > getColumnLimit(State)) {
-unsigned ExcessCharacters = State.Column - getColumnLimit(State);
-Penalty += Style.PenaltyExcessCharacter * ExcessCharacters;
-  }
+
+  unsigned Penalty =
+  handleEndOfLine(Current, State, DryRun, AllowBreak);
 
   if (Current.Role)
 Current.Role->formatFromToken(State, this, DryRun);
@@ -1282,8 +1278,10 @@
 }
 
 unsigned ContinuationIndenter::reformatRawStringLiteral(
-const FormatToken , unsigned StartColumn, LineState ,
-StringRef Delimiter, const FormatStyle , bool DryRun) {
+const FormatToken , LineState ,
+const FormatStyle , bool DryRun) {
+  unsigned StartColumn = State.Column - Current.ColumnWidth;
+  auto Delimiter = *getRawStringDelimiter(Current.TokenText);
   // The text of a raw string is between the leading 'R"delimiter(' and the
   // trailing 'delimiter)"'.
   unsigned PrefixSize = 3 + Delimiter.size();
@@ -1353,9 +1351,6 @@
 
 unsigned

[PATCH] D39900: Refactor ContinuationIndenter's breakProtrudingToken logic into slightly moreorthogonal pieces.

2017-11-14 Thread Manuel Klimek via Phabricator via cfe-commits 
klimek added a comment.

In https://reviews.llvm.org/D39900#921885, @krasimir wrote:

> Maybe we should further refactor `getRawStringStyle` into 
> `llvm::Optional> getRawStringStyleAndDelimiter` 
> and that would nicely take care of the duplicated effort?


I thought about that, too, but I'm still convinced the right solution if we 
really care about the runtime is to cache the result in FormatToken itself.


https://reviews.llvm.org/D39900



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39900: Refactor ContinuationIndenter's breakProtrudingToken logic into slightly moreorthogonal pieces.

2017-11-10 Thread Krasimir Georgiev via Phabricator via cfe-commits 
krasimir accepted this revision.
krasimir added a comment.
This revision is now accepted and ready to land.

Maybe we should further refactor `getRawStringStyle` into 
`llvm::Optional> getRawStringStyleAndDelimiter` and 
that would nicely take care of the duplicated effort?


https://reviews.llvm.org/D39900



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39900: Refactor ContinuationIndenter's breakProtrudingToken logic into slightly moreorthogonal pieces.

2017-11-10 Thread Manuel Klimek via Phabricator via cfe-commits 
klimek added inline comments.



Comment at: lib/Format/ContinuationIndenter.cpp:1028
+  unsigned Penalty =
+  handleEndOfLine(Current, State, DryRun, AllowBreak);
 

krasimir wrote:
> Why `handleEndOfLine`? Is it guaranteed that here we've reached the end of 
> the line?
No, this is used to do things in case we have reached the end of the line, as 
that's hard to check. I also wanted a better name, but wasn't able to come up 
with one (and we do similar things in other places here, where checking a 
condition is equivalent to running through all the code to do the things 
necessary).



Comment at: lib/Format/ContinuationIndenter.cpp:1284
+  unsigned StartColumn = State.Column - Current.ColumnWidth;
+  auto Delimiter = *getRawStringDelimiter(Current.TokenText);
   // The text of a raw string is between the leading 'R"delimiter(' and the

krasimir wrote:
> At this point, we compute `getRawStringDelimiter` twice: once here, and once 
> in `getRawStringStyle` in the caller side. Ideally, we'd only like to compute 
> it once. That's why it was a parameter before.
I got that, but I think passing around the delimiter is super awkward, and 
computing it is cheap. If computing it is the problem we should store it in the 
FormatToken.



Comment at: lib/Format/ContinuationIndenter.cpp:1408
+std::unique_ptr ContinuationIndenter::createBreakableToken(
+const FormatToken , LineState , bool AllowBreak) {
   unsigned StartColumn = State.Column - Current.ColumnWidth;

krasimir wrote:
> If `AllowBreak == false`, in which cases will this return a non-`nullptr`?
Only for string literals, as AllowBreak is really about whether we can break in 
the token space, but for comments (or raw strings, but those are handled 
differently anyway) we break in the comment space.



Comment at: unittests/Format/FormatTest.cpp:6322
"#include \"some long include\" // with a comment\n"
-   "#include \"some very long include 
paaath\"",
+   "#include \"some very long include path\"\n"
+   "#include \n",

krasimir wrote:
> What happened to the old test line?
It was unnecessarily long. I found it weird that it went out of its way to add 
more a's over the line limit.


https://reviews.llvm.org/D39900



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39900: Refactor ContinuationIndenter's breakProtrudingToken logic into slightly moreorthogonal pieces.

2017-11-10 Thread Manuel Klimek via Phabricator via cfe-commits 
klimek updated this revision to Diff 122433.
klimek marked an inline comment as done.
klimek added a comment.

- Add test.

Updating D39900: Refactor ContinuationIndenter's breakProtrudingToken logic 
into slightly more
==

orthogonal pieces.


https://reviews.llvm.org/D39900

Files:
  lib/Format/BreakableToken.cpp
  lib/Format/ContinuationIndenter.cpp
  lib/Format/ContinuationIndenter.h
  unittests/Format/FormatTest.cpp
  unittests/Format/FormatTestComments.cpp

Index: unittests/Format/FormatTestComments.cpp
===
--- unittests/Format/FormatTestComments.cpp
+++ unittests/Format/FormatTestComments.cpp
@@ -680,15 +680,26 @@
   EXPECT_EQ("{\n"
 "  //\n"
 "  //\\\n"
-"  // long 1 2 3 4\n"
-"  // 5\n"
+"  // long 1 2 3 4 5\n"
 "}",
 format("{\n"
"  //\n"
"  //\\\n"
"  // long 1 2 3 4 5\n"
"}",
getLLVMStyleWithColumns(20)));
+  EXPECT_EQ("{\n"
+"  //\n"
+"  //\\\n"
+"  // long 1 2 3 4 5\n"
+"  // 6\n"
+"}",
+format("{\n"
+   "  //\n"
+   "  //\\\n"
+   "  // long 1 2 3 4 5 6\n"
+   "}",
+   getLLVMStyleWithColumns(20)));
 }
 
 TEST_F(FormatTestComments, PreservesHangingIndentInCxxComments) {
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -6319,7 +6319,8 @@
"#include_next "
"#include \"abc.h\" // this is included for ABC\n"
"#include \"some long include\" // with a comment\n"
-   "#include \"some very long include paaath\"",
+   "#include \"some very long include path\"\n"
+   "#include \n",
getLLVMStyleWithColumns(35));
   EXPECT_EQ("#include \"a.h\"", format("#include  \"a.h\""));
   EXPECT_EQ("#include ", format("#include"));
Index: lib/Format/ContinuationIndenter.h
===
--- lib/Format/ContinuationIndenter.h
+++ lib/Format/ContinuationIndenter.h
@@ -29,6 +29,7 @@
 namespace format {
 
 class AnnotatedLine;
+class BreakableToken;
 struct FormatToken;
 struct LineState;
 struct ParenState;
@@ -105,11 +106,20 @@
   /// 
   /// \returns An extra penalty induced by reformatting the token.
   unsigned reformatRawStringLiteral(const FormatToken ,
-unsigned StartColumn, LineState ,
-StringRef Delimiter,
+LineState ,
 const FormatStyle ,
 bool DryRun);
 
+  /// \brief If the current token is at the end of the current line, handle
+  /// the transition to the next line.
+  unsigned handleEndOfLine(const FormatToken , LineState ,
+   bool DryRun, bool AllowBreak);
+
+  /// \brief If \p Current is a raw string that is configured to be reformatted,
+  /// return the style to be used.
+  llvm::Optional getRawStringStyle(const FormatToken ,
+const LineState );
+
   /// \brief If the current token sticks out over the end of the line, break
   /// it if possible.
   ///
@@ -120,7 +130,13 @@
   /// penalty for the column limit violation in the last line (and in single
   /// line tokens) is handled in \c addNextStateToQueue.
   unsigned breakProtrudingToken(const FormatToken , LineState ,
-bool DryRun);
+bool AllowBreak, bool DryRun);
+
+  /// \brief Returns the \c BreakableToken starting at \p Current, or nullptr
+  /// if the current token cannot be broken.
+  std::unique_ptr
+  createBreakableToken(const FormatToken , LineState ,
+   bool AllowBreak);
 
   /// \brief Appends the next token to \p State and updates information
   /// necessary for indentation.
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -1008,8 +1008,8 @@
 
   moveStatePastFakeLParens(State, Newline);
   moveStatePastScopeCloser(State);
-  bool CanBreakProtrudingToken = !State.Stack.back().NoLineBreak &&
- !State.Stack.back().NoLineBreakInOperand;
+  bool AllowBreak = !State.Stack.back().NoLineBreak &&
+!State.Stack.back().NoLineBreakInOperand;
   moveStatePastScopeOpener(State, Newline);
   moveStatePastFakeRParens(State);
 
@@ -1023,13

[PATCH] D39900: Refactor ContinuationIndenter's breakProtrudingToken logic into slightly moreorthogonal pieces.

2017-11-10 Thread Krasimir Georgiev via Phabricator via cfe-commits 
krasimir added inline comments.



Comment at: lib/Format/ContinuationIndenter.cpp:1028
+  unsigned Penalty =
+  handleEndOfLine(Current, State, DryRun, AllowBreak);
 

Why `handleEndOfLine`? Is it guaranteed that here we've reached the end of the 
line?



Comment at: lib/Format/ContinuationIndenter.cpp:1284
+  unsigned StartColumn = State.Column - Current.ColumnWidth;
+  auto Delimiter = *getRawStringDelimiter(Current.TokenText);
   // The text of a raw string is between the leading 'R"delimiter(' and the

At this point, we compute `getRawStringDelimiter` twice: once here, and once in 
`getRawStringStyle` in the caller side. Ideally, we'd only like to compute it 
once. That's why it was a parameter before.



Comment at: lib/Format/ContinuationIndenter.cpp:1408
+std::unique_ptr ContinuationIndenter::createBreakableToken(
+const FormatToken , LineState , bool AllowBreak) {
   unsigned StartColumn = State.Column - Current.ColumnWidth;

If `AllowBreak == false`, in which cases will this return a non-`nullptr`?



Comment at: unittests/Format/FormatTest.cpp:6322
"#include \"some long include\" // with a comment\n"
-   "#include \"some very long include 
paaath\"",
+   "#include \"some very long include path\"\n"
+   "#include \n",

What happened to the old test line?



Comment at: unittests/Format/FormatTestComments.cpp:683
 "  //\\\n"
-"  // long 1 2 3 4\n"
-"  // 5\n"
+"  // long 1 2 3 4 5\n"
 "}",

Please also add a test case adding a ` 6`, which actually gets broken.


https://reviews.llvm.org/D39900



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39900: Refactor ContinuationIndenter's breakProtrudingToken logic into slightly moreorthogonal pieces.

2017-11-10 Thread Manuel Klimek via Phabricator via cfe-commits 
klimek created this revision.

This patch allowed me to experiment with various alternatives to 
https://reviews.llvm.org/D33589.


https://reviews.llvm.org/D39900

Files:
  lib/Format/BreakableToken.cpp
  lib/Format/ContinuationIndenter.cpp
  lib/Format/ContinuationIndenter.h
  unittests/Format/FormatTest.cpp
  unittests/Format/FormatTestComments.cpp

Index: unittests/Format/FormatTestComments.cpp
===
--- unittests/Format/FormatTestComments.cpp
+++ unittests/Format/FormatTestComments.cpp
@@ -680,8 +680,7 @@
   EXPECT_EQ("{\n"
 "  //\n"
 "  //\\\n"
-"  // long 1 2 3 4\n"
-"  // 5\n"
+"  // long 1 2 3 4 5\n"
 "}",
 format("{\n"
"  //\n"
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -6319,7 +6319,8 @@
"#include_next "
"#include \"abc.h\" // this is included for ABC\n"
"#include \"some long include\" // with a comment\n"
-   "#include \"some very long include paaath\"",
+   "#include \"some very long include path\"\n"
+   "#include \n",
getLLVMStyleWithColumns(35));
   EXPECT_EQ("#include \"a.h\"", format("#include  \"a.h\""));
   EXPECT_EQ("#include ", format("#include"));
Index: lib/Format/ContinuationIndenter.h
===
--- lib/Format/ContinuationIndenter.h
+++ lib/Format/ContinuationIndenter.h
@@ -29,6 +29,7 @@
 namespace format {
 
 class AnnotatedLine;
+class BreakableToken;
 struct FormatToken;
 struct LineState;
 struct ParenState;
@@ -105,11 +106,20 @@
   /// 
   /// \returns An extra penalty induced by reformatting the token.
   unsigned reformatRawStringLiteral(const FormatToken ,
-unsigned StartColumn, LineState ,
-StringRef Delimiter,
+LineState ,
 const FormatStyle ,
 bool DryRun);
 
+  /// \brief If the current token is at the end of the current line, handle
+  /// the transition to the next line.
+  unsigned handleEndOfLine(const FormatToken , LineState ,
+   bool DryRun, bool AllowBreak);
+
+  /// \brief If \p Current is a raw string that is configured to be reformatted,
+  /// return the style to be used.
+  llvm::Optional getRawStringStyle(const FormatToken ,
+const LineState );
+
   /// \brief If the current token sticks out over the end of the line, break
   /// it if possible.
   ///
@@ -120,7 +130,13 @@
   /// penalty for the column limit violation in the last line (and in single
   /// line tokens) is handled in \c addNextStateToQueue.
   unsigned breakProtrudingToken(const FormatToken , LineState ,
-bool DryRun);
+bool AllowBreak, bool DryRun);
+
+  /// \brief Returns the \c BreakableToken starting at \p Current, or nullptr
+  /// if the current token cannot be broken.
+  std::unique_ptr
+  createBreakableToken(const FormatToken , LineState ,
+   bool AllowBreak);
 
   /// \brief Appends the next token to \p State and updates information
   /// necessary for indentation.
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -1008,8 +1008,8 @@
 
   moveStatePastFakeLParens(State, Newline);
   moveStatePastScopeCloser(State);
-  bool CanBreakProtrudingToken = !State.Stack.back().NoLineBreak &&
- !State.Stack.back().NoLineBreakInOperand;
+  bool AllowBreak = !State.Stack.back().NoLineBreak &&
+!State.Stack.back().NoLineBreakInOperand;
   moveStatePastScopeOpener(State, Newline);
   moveStatePastFakeRParens(State);
 
@@ -1023,13 +1023,9 @@
 
   State.Column += Current.ColumnWidth;
   State.NextToken = State.NextToken->Next;
-  unsigned Penalty = 0;
-  if (CanBreakProtrudingToken)
-Penalty = breakProtrudingToken(Current, State, DryRun);
-  if (State.Column > getColumnLimit(State)) {
-unsigned ExcessCharacters = State.Column - getColumnLimit(State);
-Penalty += Style.PenaltyExcessCharacter * ExcessCharacters;
-  }
+
+  unsigned Penalty =
+  handleEndOfLine(Current, State, DryRun, AllowBreak);
 
   if (Current.Role)
 Current.Role->formatFromToken(State, this, DryRun);
@@ -1282,8 +1278,10 @@
 }
 
 unsigned ContinuationIndenter::reformatRawStringLiteral(
-const FormatToken , unsigned StartColumn, LineState ,
-StringRef Delimiter, const FormatStyle , bool DryRun) {
+