<p dir="ltr">I absolutely agree and I will restore the old behavior soon. FWIW, you still don't have to format the whole file. Just use visual mode and select the lines with the trailing comments.. </p>
<div class="gmail_quote">On Jan 18, 2013 5:15 PM, "Nico Weber" <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Fri, Jan 18, 2013 at 12:44 AM, Daniel Jasper <<a href="mailto:djasper@google.com">djasper@google.com</a>> wrote:<br>
> Author: djasper<br>
> Date: Fri Jan 18 02:44:07 2013<br>
> New Revision: 172798<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=172798&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=172798&view=rev</a><br>
> Log:<br>
> Let the formatter align trailing line comments where possible.<br>
<br>
Before this change, the formatter produced the same output for a range<br>
of lines for the following two operations:<br>
<br>
1. hit the formatting hotkey on the first line, arrow down, hit format<br>
repeat until end<br>
2. select all lines, hit formatting hotkey<br>
<br>
This sounds like a useful property, because it means interactive<br>
formatting is just as good as formatting the whole file.<br>
<br>
After this change, this is no longer true. Operation 1 produces the<br>
"Before:" output, while operation 2 produces the "After:" output.<br>
<br>
><br>
> Before:<br>
> int a; // comment<br>
> int bbbbb; // comment<br>
><br>
> After:<br>
> int a; // comment<br>
> int bbbbb; // comment<br>
><br>
> Modified:<br>
> cfe/trunk/lib/Format/Format.cpp<br>
> cfe/trunk/unittests/Format/FormatTest.cpp<br>
><br>
> Modified: cfe/trunk/lib/Format/Format.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=172798&r1=172797&r2=172798&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=172798&r1=172797&r2=172798&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Format/Format.cpp (original)<br>
> +++ cfe/trunk/lib/Format/Format.cpp Fri Jan 18 02:44:07 2013<br>
> @@ -69,7 +69,7 @@<br>
><br>
> class AnnotatedToken {<br>
> public:<br>
> - AnnotatedToken(const FormatToken &FormatTok)<br>
> + explicit AnnotatedToken(const FormatToken &FormatTok)<br>
> : FormatTok(FormatTok), Type(TT_Unknown), SpaceRequiredBefore(false),<br>
> CanBreakBefore(false), MustBreakBefore(false),<br>
> ClosesTemplateDeclaration(false), MatchingParen(NULL), Parent(NULL) {}<br>
> @@ -117,7 +117,7 @@<br>
> for (std::list<FormatToken>::const_iterator I = ++Line.Tokens.begin(),<br>
> E = Line.Tokens.end();<br>
> I != E; ++I) {<br>
> - Current->Children.push_back(*I);<br>
> + Current->Children.push_back(AnnotatedToken(*I));<br>
> Current->Children[0].Parent = Current;<br>
> Current = &Current->Children[0];<br>
> }<br>
> @@ -189,40 +189,120 @@<br>
> unsigned PenaltyExcessCharacter;<br>
> };<br>
><br>
> -/// \brief Replaces the whitespace in front of \p Tok. Only call once for<br>
> -/// each \c FormatToken.<br>
> -static void replaceWhitespace(const AnnotatedToken &Tok, unsigned NewLines,<br>
> - unsigned Spaces, const FormatStyle &Style,<br>
> - SourceManager &SourceMgr,<br>
> - tooling::Replacements &Replaces) {<br>
> - Replaces.insert(tooling::Replacement(<br>
> - SourceMgr, Tok.FormatTok.WhiteSpaceStart, Tok.FormatTok.WhiteSpaceLength,<br>
> - std::string(NewLines, '\n') + std::string(Spaces, ' ')));<br>
> -}<br>
> -<br>
> -/// \brief Like \c replaceWhitespace, but additionally adds right-aligned<br>
> -/// backslashes to escape newlines inside a preprocessor directive.<br>
> +/// \brief Manages the whitespaces around tokens and their replacements.<br>
> ///<br>
> -/// This function and \c replaceWhitespace have the same behavior if<br>
> -/// \c Newlines == 0.<br>
> -static void replacePPWhitespace(<br>
> - const AnnotatedToken &Tok, unsigned NewLines, unsigned Spaces,<br>
> - unsigned WhitespaceStartColumn, const FormatStyle &Style,<br>
> - SourceManager &SourceMgr, tooling::Replacements &Replaces) {<br>
> - std::string NewLineText;<br>
> - if (NewLines > 0) {<br>
> - unsigned Offset = std::min<int>(Style.ColumnLimit - 1,<br>
> - WhitespaceStartColumn);<br>
> - for (unsigned i = 0; i < NewLines; ++i) {<br>
> - NewLineText += std::string(Style.ColumnLimit - Offset - 1, ' ');<br>
> - NewLineText += "\\\n";<br>
> - Offset = 0;<br>
> - }<br>
> - }<br>
> - Replaces.insert(tooling::Replacement(SourceMgr, Tok.FormatTok.WhiteSpaceStart,<br>
> - Tok.FormatTok.WhiteSpaceLength,<br>
> - NewLineText + std::string(Spaces, ' ')));<br>
> -}<br>
> +/// This includes special handling for certain constructs, e.g. the alignment of<br>
> +/// trailing line comments.<br>
> +class WhitespaceManager {<br>
> +public:<br>
> + WhitespaceManager(SourceManager &SourceMgr) : SourceMgr(SourceMgr) {}<br>
> +<br>
> + /// \brief Replaces the whitespace in front of \p Tok. Only call once for<br>
> + /// each \c AnnotatedToken.<br>
> + void replaceWhitespace(const AnnotatedToken &Tok, unsigned NewLines,<br>
> + unsigned Spaces, unsigned WhitespaceStartColumn,<br>
> + const FormatStyle &Style) {<br>
> + if (Tok.Type == TT_LineComment && NewLines < 2 &&<br>
> + (Tok.Parent != NULL || !Comments.empty())) {<br>
> + if (Style.ColumnLimit >=<br>
> + Spaces + WhitespaceStartColumn + Tok.FormatTok.TokenLength) {<br>
> + Comments.push_back(StoredComment());<br>
> + Comments.back().Tok = Tok.FormatTok;<br>
> + Comments.back().Spaces = Spaces;<br>
> + Comments.back().NewLines = NewLines;<br>
> + Comments.back().MinColumn = WhitespaceStartColumn + Spaces;<br>
> + Comments.back().MaxColumn = Style.ColumnLimit -<br>
> + Spaces - Tok.FormatTok.TokenLength;<br>
> + return;<br>
> + }<br>
> + } else if (NewLines == 0 && Tok.Children.empty() &&<br>
> + Tok.Type != TT_LineComment) {<br>
> + alignComments();<br>
> + }<br>
> + storeReplacement(Tok.FormatTok,<br>
> + std::string(NewLines, '\n') + std::string(Spaces, ' '));<br>
> + }<br>
> +<br>
> + /// \brief Like \c replaceWhitespace, but additionally adds right-aligned<br>
> + /// backslashes to escape newlines inside a preprocessor directive.<br>
> + ///<br>
> + /// This function and \c replaceWhitespace have the same behavior if<br>
> + /// \c Newlines == 0.<br>
> + void replacePPWhitespace(const AnnotatedToken &Tok, unsigned NewLines,<br>
> + unsigned Spaces, unsigned WhitespaceStartColumn,<br>
> + const FormatStyle &Style) {<br>
> + std::string NewLineText;<br>
> + if (NewLines > 0) {<br>
> + unsigned Offset = std::min<int>(Style.ColumnLimit - 1,<br>
> + WhitespaceStartColumn);<br>
> + for (unsigned i = 0; i < NewLines; ++i) {<br>
> + NewLineText += std::string(Style.ColumnLimit - Offset - 1, ' ');<br>
> + NewLineText += "\\\n";<br>
> + Offset = 0;<br>
> + }<br>
> + }<br>
> + storeReplacement(Tok.FormatTok, NewLineText + std::string(Spaces, ' '));<br>
> + }<br>
> +<br>
> + /// \brief Returns all the \c Replacements created during formatting.<br>
> + const tooling::Replacements &generateReplacements() {<br>
> + alignComments();<br>
> + return Replaces;<br>
> + }<br>
> +<br>
> +private:<br>
> + /// \brief Structure to store a comment for later layout and alignment.<br>
> + struct StoredComment {<br>
> + FormatToken Tok;<br>
> + unsigned MinColumn;<br>
> + unsigned MaxColumn;<br>
> + unsigned NewLines;<br>
> + unsigned Spaces;<br>
> + };<br>
> + SmallVector<StoredComment, 16> Comments;<br>
> + typedef SmallVector<StoredComment, 16>::iterator comment_iterator;<br>
> +<br>
> + /// \brief Try to align all stashed comments.<br>
> + void alignComments() {<br>
> + unsigned MinColumn = 0;<br>
> + unsigned MaxColumn = UINT_MAX;<br>
> + comment_iterator Start = Comments.begin();<br>
> + for (comment_iterator I = Comments.begin(), E = Comments.end(); I != E;<br>
> + ++I) {<br>
> + if (I->MinColumn > MaxColumn || I->MaxColumn < MinColumn) {<br>
> + alignComments(Start, I, MinColumn);<br>
> + MinColumn = I->MinColumn;<br>
> + MaxColumn = I->MaxColumn;<br>
> + Start = I;<br>
> + } else {<br>
> + MinColumn = std::max(MinColumn, I->MinColumn);<br>
> + MaxColumn = std::min(MaxColumn, I->MaxColumn);<br>
> + }<br>
> + }<br>
> + alignComments(Start, Comments.end(), MinColumn);<br>
> + Comments.clear();<br>
> + }<br>
> +<br>
> + /// \brief Put all the comments between \p I and \p E into \p Column.<br>
> + void alignComments(comment_iterator I, comment_iterator E, unsigned Column) {<br>
> + while (I != E) {<br>
> + unsigned Spaces = I->Spaces + Column - I->MinColumn;<br>
> + storeReplacement(I->Tok, std::string(I->NewLines, '\n') +<br>
> + std::string(Spaces, ' '));<br>
> + ++I;<br>
> + }<br>
> + }<br>
> +<br>
> + /// \brief Stores \p Text as the replacement for the whitespace in front of<br>
> + /// \p Tok.<br>
> + void storeReplacement(const FormatToken &Tok, const std::string Text) {<br>
> + Replaces.insert(tooling::Replacement(SourceMgr, Tok.WhiteSpaceStart,<br>
> + Tok.WhiteSpaceLength, Text));<br>
> + }<br>
> +<br>
> + SourceManager &SourceMgr;<br>
> + tooling::Replacements Replaces;<br>
> +};<br>
><br>
> /// \brief Returns if a token is an Objective-C selector name.<br>
> ///<br>
> @@ -238,9 +318,10 @@<br>
> UnwrappedLineFormatter(const FormatStyle &Style, SourceManager &SourceMgr,<br>
> const AnnotatedLine &Line, unsigned FirstIndent,<br>
> const AnnotatedToken &RootToken,<br>
> - tooling::Replacements &Replaces, bool StructuralError)<br>
> + WhitespaceManager &Whitespaces, bool StructuralError)<br>
> : Style(Style), SourceMgr(SourceMgr), Line(Line),<br>
> - FirstIndent(FirstIndent), RootToken(RootToken), Replaces(Replaces) {<br>
> + FirstIndent(FirstIndent), RootToken(RootToken),<br>
> + Whitespaces(Whitespaces) {<br>
> Parameters.PenaltyIndentLevel = 15;<br>
> Parameters.PenaltyLevelDecrease = 30;<br>
> Parameters.PenaltyExcessCharacter = 1000000;<br>
> @@ -464,12 +545,11 @@<br>
><br>
> if (!DryRun) {<br>
> if (!Line.InPPDirective)<br>
> - replaceWhitespace(Current.FormatTok, 1, State.Column, Style,<br>
> - SourceMgr, Replaces);<br>
> + Whitespaces.replaceWhitespace(Current, 1, State.Column,<br>
> + WhitespaceStartColumn, Style);<br>
> else<br>
> - replacePPWhitespace(Current.FormatTok, 1, State.Column,<br>
> - WhitespaceStartColumn, Style, SourceMgr,<br>
> - Replaces);<br>
> + Whitespaces.replacePPWhitespace(Current, 1, State.Column,<br>
> + WhitespaceStartColumn, Style);<br>
> }<br>
><br>
> State.Stack[ParenLevel].LastSpace = State.Column;<br>
> @@ -485,7 +565,7 @@<br>
> Spaces = Style.SpacesBeforeTrailingComments;<br>
><br>
> if (!DryRun)<br>
> - replaceWhitespace(Current, 0, Spaces, Style, SourceMgr, Replaces);<br>
> + Whitespaces.replaceWhitespace(Current, 0, Spaces, State.Column, Style);<br>
><br>
> // FIXME: Do we need to do this for assignments nested in other<br>
> // expressions?<br>
> @@ -719,7 +799,7 @@<br>
> const AnnotatedLine &Line;<br>
> const unsigned FirstIndent;<br>
> const AnnotatedToken &RootToken;<br>
> - tooling::Replacements &Replaces;<br>
> + WhitespaceManager &Whitespaces;<br>
><br>
> // A map from an indent state to a pair (Result, Used-StopAt).<br>
> typedef std::map<LineState, std::pair<unsigned, unsigned> > StateMap;<br>
> @@ -1564,7 +1644,7 @@<br>
> SourceManager &SourceMgr,<br>
> const std::vector<CharSourceRange> &Ranges)<br>
> : Diag(Diag), Style(Style), Lex(Lex), SourceMgr(SourceMgr),<br>
> - Ranges(Ranges) {}<br>
> + Whitespaces(SourceMgr), Ranges(Ranges) {}<br>
><br>
> virtual ~Formatter() {}<br>
><br>
> @@ -1587,7 +1667,7 @@<br>
> PreviousEndOfLineColumn);<br>
> tryFitMultipleLinesInOne(Indent, I, E);<br>
> UnwrappedLineFormatter Formatter(Style, SourceMgr, TheLine, Indent,<br>
> - TheLine.First, Replaces,<br>
> + TheLine.First, Whitespaces,<br>
> StructuralError);<br>
> PreviousEndOfLineColumn = Formatter.format();<br>
> } else {<br>
> @@ -1602,7 +1682,7 @@<br>
> 1;<br>
> }<br>
> }<br>
> - return Replaces;<br>
> + return Whitespaces.generateReplacements();<br>
> }<br>
><br>
> private:<br>
> @@ -1789,10 +1869,10 @@<br>
> static_cast<int>(Indent) + Style.AccessModifierOffset >= 0)<br>
> Indent += Style.AccessModifierOffset;<br>
> if (!InPPDirective || Tok.HasUnescapedNewline) {<br>
> - replaceWhitespace(Tok, Newlines, Indent, Style, SourceMgr, Replaces);<br>
> + Whitespaces.replaceWhitespace(RootToken, Newlines, Indent, 0, Style);<br>
> } else {<br>
> - replacePPWhitespace(Tok, Newlines, Indent, PreviousEndOfLineColumn, Style,<br>
> - SourceMgr, Replaces);<br>
> + Whitespaces.replacePPWhitespace(RootToken, Newlines, Indent,<br>
> + PreviousEndOfLineColumn, Style);<br>
> }<br>
> return Indent;<br>
> }<br>
> @@ -1801,7 +1881,7 @@<br>
> FormatStyle Style;<br>
> Lexer &Lex;<br>
> SourceManager &SourceMgr;<br>
> - tooling::Replacements Replaces;<br>
> + WhitespaceManager Whitespaces;<br>
> std::vector<CharSourceRange> Ranges;<br>
> std::vector<AnnotatedLine> AnnotatedLines;<br>
> bool StructuralError;<br>
><br>
> Modified: cfe/trunk/unittests/Format/FormatTest.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=172798&r1=172797&r2=172798&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=172798&r1=172797&r2=172798&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/unittests/Format/FormatTest.cpp (original)<br>
> +++ cfe/trunk/unittests/Format/FormatTest.cpp Fri Jan 18 02:44:07 2013<br>
> @@ -352,8 +352,8 @@<br>
> verifyFormat("void f() {\n"<br>
> " // Doesn't do anything\n"<br>
> "}");<br>
> - verifyFormat("void f(int i, // some comment (probably for i)\n"<br>
> - " int j, // some comment (probably for j)\n"<br>
> + verifyFormat("void f(int i, // some comment (probably for i)\n"<br>
> + " int j, // some comment (probably for j)\n"<br>
> " int k); // some comment (probably for k)");<br>
> verifyFormat("void f(int i,\n"<br>
> " // some comment (probably for j)\n"<br>
> @@ -361,8 +361,32 @@<br>
> " // some comment (probably for k)\n"<br>
> " int k);");<br>
><br>
> - verifyFormat("int i // This is a fancy variable\n"<br>
> - " = 5;");<br>
> + verifyFormat("int i // This is a fancy variable\n"<br>
> + " = 5; // with nicely aligned comment.");<br>
> +<br>
> + verifyFormat("// Leading comment.\n"<br>
> + "int a; // Trailing comment.");<br>
> + verifyFormat("int a; // Trailing comment\n"<br>
> + " // on 2\n"<br>
> + " // or 3 lines.\n"<br>
> + "int b;");<br>
> + verifyFormat("int a; // Trailing comment\n"<br>
> + "\n"<br>
> + "// Leading comment.\n"<br>
> + "int b;");<br>
> + verifyFormat("int a; // Comment.\n"<br>
> + " // More details.\n"<br>
> + "int bbbb; // Another comment.");<br>
> + verifyFormat(<br>
> + "int aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa; // comment\n"<br>
> + "int bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb; // comment\n"<br>
> + "int cccccccccccccccccccccccccccccc; // comment\n"<br>
> + "int ddd; // looooooooooooooooooooooooong comment\n"<br>
> + "int aaaaaaaaaaaaaaaaaaaaaaa; // comment\n"<br>
> + "int bbbbbbbbbbbbbbbbbbbbb; // comment\n"<br>
> + "int ccccccccccccccccccc; // comment");<br>
> +<br>
> +<br>
><br>
> verifyFormat("enum E {\n"<br>
> " // comment\n"<br>
> @@ -835,7 +859,7 @@<br>
> " aaaaaaaaaaaaaaaaaaaaat(aaaaaaaaaaaaaaaaaaaaaaaaaaaa) {}");<br>
><br>
> verifyGoogleFormat("MyClass::MyClass(int var)\n"<br>
> - " : some_var_(var), // 4 space indent\n"<br>
> + " : some_var_(var), // 4 space indent\n"<br>
> " some_other_var_(var + 1) { // lined up\n"<br>
> "}");<br>
><br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>