[PATCH] D40310: Restructure how we break tokens.

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added a comment.

Here're a few nits. I'll need an evening to review the meaty part :)



================
Comment at: lib/Format/BreakableToken.cpp:73
+  if (ColumnLimit <= ContentStartColumn + 1) {
     return BreakableToken::Split(StringRef::npos, 0);
+  }
----------------
nit: no braces around single-statement if body.


================
Comment at: lib/Format/BreakableToken.cpp:198
+         "Getting the length of a part of the string literal indicates that "
+         "the code tries to reflow it.");
+  return UnbreakableTailLength + Postfix.size() +
----------------
How about clients that explicitly pass `Length = Line.size() - Offset`?


================
Comment at: lib/Format/BreakableToken.cpp:476
+                                    StartColumn, Style.TabWidth, Encoding);
+  // FIXME: Test that this is handled correctly for Length != npos!
+  // The last line gets a "*/" postfAix.
----------------
Could you elaborate?


================
Comment at: lib/Format/BreakableToken.cpp:477
+  // FIXME: Test that this is handled correctly for Length != npos!
+  // The last line gets a "*/" postfAix.
   if (LineIndex + 1 == Lines.size()) {
----------------
nit: postfix.


================
Comment at: lib/Format/BreakableToken.cpp:566
     if (DelimitersOnNewline) {
       // Since we're breaking af index 1 below, the break position and the
       // break length are the same.
----------------
nit: af -> at


================
Comment at: lib/Format/BreakableToken.cpp:570
       if (BreakLength != StringRef::npos) {
         insertBreak(LineIndex, 0, Split(1, BreakLength), Whitespaces);
       }
----------------
nit: no braces around single-statement if bodies.


================
Comment at: lib/Format/BreakableToken.cpp:851
   }
+  // FIXME: Decide whether we want to reflow non-regular indents.
   return LineIndex > 0 && !CommentPragmasRegex.match(IndentContent) &&
----------------
could you give an example of what's a non-regular indent?


================
Comment at: lib/Format/BreakableToken.h:52
+///
+/// The mechanism to adapt the layout of the breakable token are organised
+/// around the concept of a \c Split, which is a whitespace range that signifies
----------------
Replace `are` with `is`.


================
Comment at: lib/Format/BreakableToken.h:100
+  /// \brief Returns the number of columns required to format the range at bytes
+  /// \p Offset to \p Offset \c + \p Length.
+  ///
----------------
Does this include the byte `Offset + Length`?


================
Comment at: lib/Format/BreakableToken.h:107
   ///
-  /// Note that previous breaks are not taken into account. \p TailOffset is
-  /// always specified from the start of the (original) line.
-  /// \p Length can be set to StringRef::npos, which means "to the end of line".
-  virtual unsigned
-  getLineLengthAfterSplit(unsigned LineIndex, unsigned TailOffset,
-                          StringRef::size_type Length) const = 0;
+  /// \p StartColumn is the column at which the text starts, needed to compute
+  ///    tab stops correctly.
----------------
`text` is ambiguous here: does it refer to the content of the line or to the range defined by the offset and length?


================
Comment at: lib/Format/BreakableToken.h:114
+  /// \brief Returns the column at which content in line \p LineIndex starts,
+  /// assuming no reflow.
+  virtual unsigned getContentStartColumn(unsigned LineIndex,
----------------
What is `Break` used for?


================
Comment at: lib/Format/BreakableToken.h:120
   /// \p LineIndex, if previously broken at \p TailOffset. If possible, do not
   /// violate \p ColumnLimit.
   virtual Split getSplit(unsigned LineIndex, unsigned TailOffset,
----------------
What is `ReflownColumn` used for?


================
Comment at: lib/Format/BreakableToken.h:142
   /// \brief Returns a whitespace range (offset, length) of the content at
-  /// \p LineIndex such that the content preceding this range needs to be
-  /// reformatted before any breaks are made to this line.
+  /// \p LineIndex such that the content of the current line is reflown to the
+  /// end of the previous one.
----------------
Does the current line refer to the line at LineIndex?


================
Comment at: lib/Format/ContinuationIndenter.cpp:1504
                                  : Style.PenaltyBreakComment;
-  unsigned RemainingSpace = ColumnLimit - Current.UnbreakableTailLength;
+  // Stores whether we introduce a break anywhere in the token.
   bool BreakInserted = Token->introducesBreakBeforeToken();
----------------
Does a reflow count as a break?


================
Comment at: unittests/Format/FormatTest.cpp:7735
 
+TEST_F(FormatTest, BreaksStringLiteralsTODO) {
+  EXPECT_EQ("C a = \"some more \"\n"
----------------
TODO?


================
Comment at: unittests/Format/FormatTest.cpp:10603
             format("\"一\t二 \t三 四 五\t六 \t七 八九十\tqq\"",
                    getLLVMStyleWithColumns(11)));
 
----------------
What happened here?


================
Comment at: unittests/Format/FormatTestComments.cpp:1104
+            " * doesn't fit on\n"
+            " * one line.  */",
             format("/* "
----------------
I think this is desirable. The jsdoc folks really wanted to fix-up the `*/` at the end. I think it has to do with `DelimitersOnNewline`.
If this already works in Java and js mode, that could be good enough.


================
Comment at: unittests/Format/FormatTestComments.cpp:2108
+  // FIXME: This assumes we do not continue compressing whitespace once we are
+  // in reflow mode. Consider compressing whitespace.
+
----------------
I thinks that we should be compressing whitespace in reflow mode too.


================
Comment at: unittests/Format/FormatTestComments.cpp:2149
+  // to keep this?
+  EXPECT_EQ("// some text\n"
+            "// that\n"
----------------
This is like this for cases like lists in comments:
```
blah-blah-blah:
  1. blah
  2. blah-blah
```
I think here the block comments behavior might be wrong.


https://reviews.llvm.org/D40310