[cfe-commits] r170457 - in /cfe/trunk: lib/Format/Format.cpp unittests/Format/FormatTest.cpp
Daniel Jasper
djasper at google.com
Wed Dec 19 00:18:58 PST 2012
As for Chromium and WebKit coding styles, I am already looking at them
specifically, as they will require some things that are much easier in
Google/LLVM. As for configuration, the coding styles are currently
hard-coded mostly because it is easiest, makes clang-format easy to use and
avoids configuration files running out of sync with the clang-format
version. Once we are a bit further along (maybe soon-ish), I agree that
config files are a better solution at least as an additional option.
On Tue, Dec 18, 2012 at 10:45 PM, Nico Weber <thakis at chromium.org> wrote:
> Out of interest, what's the plan for adding more coding styles? It looks
> like there's an coding style enum somewhere; are projects who don't use one
> of the existing coding styles expected to contribute patches to clang
> itself for their styles? Or will there be some config file eventually?
>
> I'm asking because webkit style for initializers prefers one line per
> initializer (and leading commas):
> http://www.webkit.org/coding/coding-style.html#punctuation
>
> Nico
>
> On Tue, Dec 18, 2012 at 1:05 PM, Daniel Jasper <djasper at google.com> wrote:
>
>> Author: djasper
>> Date: Tue Dec 18 15:05:13 2012
>> New Revision: 170457
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=170457&view=rev
>> Log:
>> Better support for constructor initializers.
>>
>> We used to format initializers like this (with a sort of hacky
>> implementation):
>> Constructor()
>> : Val1(A),
>> Val2(B) {
>>
>> and now format like this (with a somewhat better solution):
>> Constructor()
>> : Val1(A), Val2(B) {
>>
>> assuming this would not fit on a single line. Also added tests.
>>
>> As a side effect we now first analyze whether an UnwrappedLine needs to be
>> split at all. If not, not splitting it is the best solution by
>> definition. As
>> this should be a very common case in normal code, not exploring the entire
>> solution space can provide significant speedup.
>>
>> 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=170457&r1=170456&r2=170457&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/Format/Format.cpp (original)
>> +++ cfe/trunk/lib/Format/Format.cpp Tue Dec 18 15:05:13 2012
>> @@ -37,6 +37,7 @@
>> TT_OverloadedOperator,
>> TT_PointerOrReference,
>> TT_ConditionalExpr,
>> + TT_CtorInitializerColon,
>> TT_LineComment,
>> TT_BlockComment
>> };
>> @@ -73,7 +74,6 @@
>> }
>>
>> struct OptimizationParameters {
>> - unsigned PenaltyExtraLine;
>> unsigned PenaltyIndentLevel;
>> };
>>
>> @@ -83,13 +83,9 @@
>> const UnwrappedLine &Line,
>> const std::vector<TokenAnnotation> &Annotations,
>> tooling::Replacements &Replaces, bool
>> StructuralError)
>> - : Style(Style),
>> - SourceMgr(SourceMgr),
>> - Line(Line),
>> - Annotations(Annotations),
>> - Replaces(Replaces),
>> + : Style(Style), SourceMgr(SourceMgr), Line(Line),
>> + Annotations(Annotations), Replaces(Replaces),
>> StructuralError(StructuralError) {
>> - Parameters.PenaltyExtraLine = 100;
>> Parameters.PenaltyIndentLevel = 5;
>> }
>>
>> @@ -100,8 +96,6 @@
>> // Initialize state dependent on indent.
>> IndentState State;
>> State.Column = Indent;
>> - State.CtorInitializerOnNewLine = false;
>> - State.InCtorInitializer = false;
>> State.ConsumedTokens = 0;
>> State.Indent.push_back(Indent + 4);
>> State.LastSpace.push_back(Indent);
>> @@ -110,11 +104,33 @@
>> // The first token has already been indented and thus consumed.
>> moveStateToNextToken(State);
>>
>> + // Check whether the UnwrappedLine can be put onto a single line. If
>> so,
>> + // this is bound to be the optimal solution (by definition) and we
>> don't
>> + // need to analyze the entire solution space.
>> + unsigned Columns = State.Column;
>> + bool FitsOnALine = true;
>> + for (unsigned i = 1, n = Line.Tokens.size(); i != n; ++i) {
>> + Columns += (Annotations[i].SpaceRequiredBefore ? 1 : 0) +
>> + Line.Tokens[i].Tok.getLength();
>> + // A special case for the colon of a constructor initializer as
>> this only
>> + // needs to be put on a new line if the line needs to be split.
>> + if (Columns > Style.ColumnLimit ||
>> + (Annotations[i].MustBreakBefore &&
>> + Annotations[i].Type !=
>> TokenAnnotation::TT_CtorInitializerColon)) {
>> + FitsOnALine = false;
>> + break;
>> + }
>> + }
>> +
>> // Start iterating at 1 as we have correctly formatted of Token #0
>> above.
>> for (unsigned i = 1, n = Line.Tokens.size(); i != n; ++i) {
>> - unsigned NoBreak = calcPenalty(State, false, UINT_MAX);
>> - unsigned Break = calcPenalty(State, true, NoBreak);
>> - addTokenToState(Break < NoBreak, false, State);
>> + if (FitsOnALine) {
>> + addTokenToState(false, false, State);
>> + } else {
>> + unsigned NoBreak = calcPenalty(State, false, UINT_MAX);
>> + unsigned Break = calcPenalty(State, true, NoBreak);
>> + addTokenToState(Break < NoBreak, false, State);
>> + }
>> }
>> }
>>
>> @@ -146,9 +162,6 @@
>> /// on a level.
>> std::vector<unsigned> FirstLessLess;
>>
>> - bool CtorInitializerOnNewLine;
>> - bool InCtorInitializer;
>> -
>> /// \brief Comparison operator to be able to used \c IndentState in
>> \c map.
>> bool operator<(const IndentState &Other) const {
>> if (Other.ConsumedTokens != ConsumedTokens)
>> @@ -212,11 +225,8 @@
>>
>> State.LastSpace[ParenLevel] = State.Indent[ParenLevel];
>> if (Current.Tok.is(tok::colon) &&
>> - Annotations[Index].Type !=
>> TokenAnnotation::TT_ConditionalExpr) {
>> + Annotations[Index].Type != TokenAnnotation::TT_ConditionalExpr)
>> State.Indent[ParenLevel] += 2;
>> - State.CtorInitializerOnNewLine = true;
>> - State.InCtorInitializer = true;
>> - }
>> } else {
>> unsigned Spaces = Annotations[Index].SpaceRequiredBefore ? 1 : 0;
>> if (Annotations[Index].Type == TokenAnnotation::TT_LineComment)
>> @@ -228,10 +238,7 @@
>> if (Previous.Tok.is(tok::l_paren) ||
>> Annotations[Index - 1].Type ==
>> TokenAnnotation::TT_TemplateOpener)
>> State.Indent[ParenLevel] = State.Column;
>> - if (Current.Tok.is(tok::colon)) {
>> - State.Indent[ParenLevel] = State.Column + 3;
>> - State.InCtorInitializer = true;
>> - }
>> +
>> // Top-level spaces are exempt as that mostly leads to better
>> results.
>> State.Column += Spaces;
>> if (Spaces > 0 && ParenLevel != 0)
>> @@ -279,10 +286,8 @@
>> "Tried to calculate penalty for splitting after the last
>> token");
>> const FormatToken &Left = Line.Tokens[Index];
>> const FormatToken &Right = Line.Tokens[Index + 1];
>> - if (Left.Tok.is(tok::semi))
>> + if (Left.Tok.is(tok::semi) || Left.Tok.is(tok::comma))
>> return 0;
>> - if (Left.Tok.is(tok::comma))
>> - return 1;
>> if (Left.Tok.is(tok::equal) || Left.Tok.is(tok::l_paren) ||
>> Left.Tok.is(tok::pipepipe) || Left.Tok.is(tok::ampamp))
>> return 2;
>> @@ -313,18 +318,10 @@
>> if (NewLine && !Annotations[State.ConsumedTokens].CanBreakBefore)
>> return UINT_MAX;
>>
>> - if (State.ConsumedTokens > 0 && !NewLine &&
>> - State.CtorInitializerOnNewLine &&
>> - Line.Tokens[State.ConsumedTokens - 1].Tok.is(tok::comma))
>> - return UINT_MAX;
>> -
>> - if (NewLine && State.InCtorInitializer &&
>> !State.CtorInitializerOnNewLine)
>> - return UINT_MAX;
>> -
>> unsigned CurrentPenalty = 0;
>> if (NewLine) {
>> CurrentPenalty += Parameters.PenaltyIndentLevel *
>> State.Indent.size() +
>> - Parameters.PenaltyExtraLine +
>> splitPenalty(State.ConsumedTokens - 1);
>> + splitPenalty(State.ConsumedTokens - 1);
>> }
>>
>> addTokenToState(NewLine, true, State);
>> @@ -413,9 +410,7 @@
>> public:
>> TokenAnnotator(const UnwrappedLine &Line, const FormatStyle &Style,
>> SourceManager &SourceMgr)
>> - : Line(Line),
>> - Style(Style),
>> - SourceMgr(SourceMgr) {
>> + : Line(Line), Style(Style), SourceMgr(SourceMgr) {
>> }
>>
>> /// \brief A parser that gathers additional information about tokens.
>> @@ -427,9 +422,7 @@
>> public:
>> AnnotatingParser(const SmallVector<FormatToken, 16> &Tokens,
>> std::vector<TokenAnnotation> &Annotations)
>> - : Tokens(Tokens),
>> - Annotations(Annotations),
>> - Index(0) {
>> + : Tokens(Tokens), Annotations(Annotations), Index(0) {
>> }
>>
>> bool parseAngle() {
>> @@ -496,6 +489,10 @@
>> switch (Tokens[CurrentIndex].Tok.getKind()) {
>> case tok::l_paren:
>> parseParens();
>> + if (Index < Tokens.size() && Tokens[Index].Tok.is(tok::colon)) {
>> + Annotations[Index].Type =
>> TokenAnnotation::TT_CtorInitializerColon;
>> + next();
>> + }
>> break;
>> case tok::l_square:
>> parseSquare();
>> @@ -557,7 +554,10 @@
>> Annotation.CanBreakBefore =
>> canBreakBetween(Line.Tokens[i - 1], Line.Tokens[i]);
>>
>> - if (Line.Tokens[i].Tok.is(tok::colon)) {
>> + if (Annotation.Type == TokenAnnotation::TT_CtorInitializerColon) {
>> + Annotation.MustBreakBefore = true;
>> + Annotation.SpaceRequiredBefore = true;
>> + } else if (Line.Tokens[i].Tok.is(tok::colon)) {
>> Annotation.SpaceRequiredBefore =
>> Line.Tokens[0].Tok.isNot(tok::kw_case) && i != e - 1;
>> } else if (Annotations[i - 1].Type ==
>> TokenAnnotation::TT_UnaryOperator) {
>> @@ -769,9 +769,7 @@
>> class LexerBasedFormatTokenSource : public FormatTokenSource {
>> public:
>> LexerBasedFormatTokenSource(Lexer &Lex, SourceManager &SourceMgr)
>> - : GreaterStashed(false),
>> - Lex(Lex),
>> - SourceMgr(SourceMgr),
>> + : GreaterStashed(false), Lex(Lex), SourceMgr(SourceMgr),
>> IdentTable(Lex.getLangOpts()) {
>> Lex.SetKeepWhitespaceMode(true);
>> }
>> @@ -831,10 +829,7 @@
>> public:
>> Formatter(const FormatStyle &Style, Lexer &Lex, SourceManager
>> &SourceMgr,
>> const std::vector<CharSourceRange> &Ranges)
>> - : Style(Style),
>> - Lex(Lex),
>> - SourceMgr(SourceMgr),
>> - Ranges(Ranges),
>> + : Style(Style), Lex(Lex), SourceMgr(SourceMgr), Ranges(Ranges),
>> StructuralError(false) {
>> }
>>
>>
>> Modified: cfe/trunk/unittests/Format/FormatTest.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=170457&r1=170456&r2=170457&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/unittests/Format/FormatTest.cpp (original)
>> +++ cfe/trunk/unittests/Format/FormatTest.cpp Tue Dec 18 15:05:13 2012
>> @@ -374,6 +374,41 @@
>> " parameter, parameter, parameter)),
>> SecondLongCall(parameter));");
>> }
>>
>> +TEST_F(FormatTest, ConstructorInitializers) {
>> + verifyFormat("Constructor() : Initializer(FitsOnTheLine) {\n}");
>> +
>> + verifyFormat(
>> + "SomeClass::Constructor()\n"
>> + " : aaaaaaaaaaaaa(aaaaaaaaaaaaaa),
>> aaaaaaaaaaaaaaa(aaaaaaaaaaaa) {\n"
>> + "}");
>> +
>> + verifyFormat(
>> + "SomeClass::Constructor()\n"
>> + " :
>> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa),\n"
>> + " aaaaaaaaaaaaaaa(aaaaaaaaaaaa) {\n"
>> + "}");
>> +
>> + verifyFormat("Constructor()\n"
>> + " :
>> aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaa),\n"
>> + "
>> aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaa,\n"
>> + "
>> aaaaaaaaaaaaaaaaaaaaaaaaaaa),\n"
>> + " aaaaaaaaaaaaaaaaaaaaaaa() {\n"
>> + "}");
>> +
>> + // Here a line could be saved by splitting the second initializer onto
>> two
>> + // lines, but that is not desireable.
>> + verifyFormat("Constructor()\n"
>> + " :
>> aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaa),\n"
>> + " aaaaaaaaaaa(aaaaaaaaaaa),\n"
>> + "
>> aaaaaaaaaaaaaaaaaaaaat(aaaaaaaaaaaaaaaaaaaaaaaaaaaa) {\n"
>> + "}");
>> +
>> + verifyGoogleFormat("MyClass::MyClass(int var)\n"
>> + " : some_var_(var), // 4 space indent\n"
>> + " some_other_var_(var + 1) { // lined up\n"
>> + "}");
>> +}
>> +
>> TEST_F(FormatTest, BreaksAsHighAsPossible) {
>> verifyFormat(
>> "if ((aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&
>> aaaaaaaaaaaaaaaaaaaaaaaaaa) ||\n"
>> @@ -461,13 +496,11 @@
>> }
>>
>> TEST_F(FormatTest, WrapsAtFunctionCallsIfNecessary) {
>> - verifyFormat(
>> - "LoooooooooooooooooooooooooooooooooooooongObject\n"
>> - " .looooooooooooooooooooooooooooooooooooooongFunction();");
>> + verifyFormat("LoooooooooooooooooooooooooooooooooooooongObject\n"
>> + "
>> .looooooooooooooooooooooooooooooooooooooongFunction();");
>>
>> - verifyFormat(
>> - "LoooooooooooooooooooooooooooooooooooooongObject\n"
>> - " ->looooooooooooooooooooooooooooooooooooooongFunction();");
>> + verifyFormat("LoooooooooooooooooooooooooooooooooooooongObject\n"
>> + "
>> ->looooooooooooooooooooooooooooooooooooooongFunction();");
>>
>> verifyFormat(
>>
>> "LooooooooooooooooooooooooooooooooongObject->shortFunction(Parameter1,\n"
>> @@ -485,10 +518,9 @@
>> "function(LoooooooooooooooooooooooooooooooooooongObject\n"
>> "
>> ->loooooooooooooooooooooooooooooooooooooooongFunction());");
>>
>> - verifyFormat(
>> - "if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaa) ||\n"
>> - " aaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {\n"
>> - "}");
>> + verifyFormat("if
>> (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaa) ||\n"
>> + " aaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {\n"
>> + "}");
>> }
>>
>> TEST_F(FormatTest, UnderstandsTemplateParameters) {
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121219/66ee7e4f/attachment.html>
More information about the cfe-commits
mailing list