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