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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits