r175432 - Prevent line breaks that make stuff hard to read.
Daniel Jasper
djasper at google.com
Tue Feb 19 01:30:14 PST 2013
Thanks for catching this. I think the only bug was the ">" instead of "!=".
However, cleaning this up makes a lot of sense. Submitted cleanup in
r175500.
On Tue, Feb 19, 2013 at 2:30 AM, Aaron Ballman <aaron at aaronballman.com>wrote:
> I'm not 100% certain that this is the commit which caused the breakage
> I am seeing, but operator< causes assertions to fail in MSVC's
> predicate debuggers. Long story short, for some LineStates,
> !pred(left, right) fails, and pred(right, left) succeeds which causes
> an assertion.
>
> One thing which struck me as potentially odd was that the code was
> using Other > this in some places, and Other < this in other places.
> When I flip the logic to be consistent, all of the assertions quieted
> down. I've attached a patch showing the changes I've made, but I'd
> like some oversight to make sure I've not mucked things up.
>
> Thanks!
>
> ~Aaron
>
> On Mon, Feb 18, 2013 at 6:05 AM, Daniel Jasper <djasper at google.com> wrote:
> > Author: djasper
> > Date: Mon Feb 18 05:05:07 2013
> > New Revision: 175432
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=175432&view=rev
> > Log:
> > Prevent line breaks that make stuff hard to read.
> >
> > Before:
> > aaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa).aaaaaaaaaaaaaaaaa(
> > aaaaaaaaaaaaaaaaaaaaa).aaaaaaaaaaaaaaaaaaa(
> > aaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaa,
> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);
> >
> > After:
> > aaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa)
> > .aaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa)
> > .aaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaa,
> > aaaaaaaaaaaaaaaaaaa,
> > aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);
> >
> > 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=175432&r1=175431&r2=175432&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/lib/Format/Format.cpp (original)
> > +++ cfe/trunk/lib/Format/Format.cpp Mon Feb 18 05:05:07 2013
> > @@ -251,6 +251,7 @@ public:
> > State.VariablePos = 0;
> > State.LineContainsContinuedForLoopSection = false;
> > State.ParenLevel = 0;
> > + State.StartOfLineLevel = State.ParenLevel;
> >
> > DEBUG({
> > DebugTokenState(*State.NextToken);
> > @@ -384,6 +385,9 @@ private:
> > /// \brief The level of nesting inside (), [], <> and {}.
> > unsigned ParenLevel;
> >
> > + /// \brief The \c ParenLevel at the start of this line.
> > + unsigned StartOfLineLevel;
> > +
> > /// \brief A stack keeping track of properties applying to
> parenthesis
> > /// levels.
> > std::vector<ParenState> Stack;
> > @@ -401,6 +405,8 @@ private:
> > return LineContainsContinuedForLoopSection;
> > if (Other.ParenLevel != ParenLevel)
> > return Other.ParenLevel < ParenLevel;
> > + if (Other.StartOfLineLevel < StartOfLineLevel)
> > + return Other.StartOfLineLevel < StartOfLineLevel;
> > return Other.Stack < Stack;
> > }
> > };
> > @@ -489,6 +495,7 @@ private:
> > }
> >
> > State.Stack.back().LastSpace = State.Column;
> > + State.StartOfLineLevel = State.ParenLevel;
> > if (Current.is(tok::colon) && Current.Type != TT_ConditionalExpr)
> > State.Stack.back().Indent += 2;
> > } else {
> > @@ -772,6 +779,14 @@ private:
> > !(State.NextToken->is(tok::r_brace) &&
> > State.Stack.back().BreakBeforeClosingBrace))
> > return false;
> > + // This prevents breaks like:
> > + // ...
> > + // SomeParameter, OtherParameter).DoSomething(
> > + // ...
> > + // As they hide "DoSomething" and generally bad for readability.
> > + if (State.NextToken->Parent->is(tok::l_paren) &&
> > + State.ParenLevel <= State.StartOfLineLevel)
> > + return false;
> > // Trying to insert a parameter on a new line if there are already
> more than
> > // one parameter on the current line is bin packing.
> > if (State.Stack.back().HasMultiParameterLine &&
> >
> > Modified: cfe/trunk/unittests/Format/FormatTest.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=175432&r1=175431&r2=175432&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/unittests/Format/FormatTest.cpp (original)
> > +++ cfe/trunk/unittests/Format/FormatTest.cpp Mon Feb 18 05:05:07 2013
> > @@ -1457,6 +1457,13 @@ TEST_F(FormatTest, WrapsAtFunctionCallsI
> > verifyFormat("SomeMap[std::pair(aaaaaaaaaaaa, bbbbbbbbbbbbbbb)]\n"
> > " .insert(ccccccccccccccccccccccc);");
> >
> > + verifyGoogleFormat(
> > + "aaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa)\n"
> > + " .aaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa)\n"
> > + " .aaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaa,\n"
> > + " aaaaaaaaaaaaaaaaaaa,\n"
> > + " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);");
> > +
> > // Here, it is not necessary to wrap at "." or "->".
> > verifyFormat("if
> (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaa) ||\n"
> > " aaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {\n}");
> >
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
> On Mon, Feb 18, 2013 at 6:05 AM, Daniel Jasper <djasper at google.com> wrote:
> > Author: djasper
> > Date: Mon Feb 18 05:05:07 2013
> > New Revision: 175432
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=175432&view=rev
> > Log:
> > Prevent line breaks that make stuff hard to read.
> >
> > Before:
> > aaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa).aaaaaaaaaaaaaaaaa(
> > aaaaaaaaaaaaaaaaaaaaa).aaaaaaaaaaaaaaaaaaa(
> > aaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaa,
> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);
> >
> > After:
> > aaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa)
> > .aaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa)
> > .aaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaa,
> > aaaaaaaaaaaaaaaaaaa,
> > aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);
> >
> > 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=175432&r1=175431&r2=175432&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/lib/Format/Format.cpp (original)
> > +++ cfe/trunk/lib/Format/Format.cpp Mon Feb 18 05:05:07 2013
> > @@ -251,6 +251,7 @@ public:
> > State.VariablePos = 0;
> > State.LineContainsContinuedForLoopSection = false;
> > State.ParenLevel = 0;
> > + State.StartOfLineLevel = State.ParenLevel;
> >
> > DEBUG({
> > DebugTokenState(*State.NextToken);
> > @@ -384,6 +385,9 @@ private:
> > /// \brief The level of nesting inside (), [], <> and {}.
> > unsigned ParenLevel;
> >
> > + /// \brief The \c ParenLevel at the start of this line.
> > + unsigned StartOfLineLevel;
> > +
> > /// \brief A stack keeping track of properties applying to
> parenthesis
> > /// levels.
> > std::vector<ParenState> Stack;
> > @@ -401,6 +405,8 @@ private:
> > return LineContainsContinuedForLoopSection;
> > if (Other.ParenLevel != ParenLevel)
> > return Other.ParenLevel < ParenLevel;
> > + if (Other.StartOfLineLevel < StartOfLineLevel)
> > + return Other.StartOfLineLevel < StartOfLineLevel;
> > return Other.Stack < Stack;
> > }
> > };
> > @@ -489,6 +495,7 @@ private:
> > }
> >
> > State.Stack.back().LastSpace = State.Column;
> > + State.StartOfLineLevel = State.ParenLevel;
> > if (Current.is(tok::colon) && Current.Type != TT_ConditionalExpr)
> > State.Stack.back().Indent += 2;
> > } else {
> > @@ -772,6 +779,14 @@ private:
> > !(State.NextToken->is(tok::r_brace) &&
> > State.Stack.back().BreakBeforeClosingBrace))
> > return false;
> > + // This prevents breaks like:
> > + // ...
> > + // SomeParameter, OtherParameter).DoSomething(
> > + // ...
> > + // As they hide "DoSomething" and generally bad for readability.
> > + if (State.NextToken->Parent->is(tok::l_paren) &&
> > + State.ParenLevel <= State.StartOfLineLevel)
> > + return false;
> > // Trying to insert a parameter on a new line if there are already
> more than
> > // one parameter on the current line is bin packing.
> > if (State.Stack.back().HasMultiParameterLine &&
> >
> > Modified: cfe/trunk/unittests/Format/FormatTest.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=175432&r1=175431&r2=175432&view=diff
> >
> ==============================================================================
> > --- cfe/trunk/unittests/Format/FormatTest.cpp (original)
> > +++ cfe/trunk/unittests/Format/FormatTest.cpp Mon Feb 18 05:05:07 2013
> > @@ -1457,6 +1457,13 @@ TEST_F(FormatTest, WrapsAtFunctionCallsI
> > verifyFormat("SomeMap[std::pair(aaaaaaaaaaaa, bbbbbbbbbbbbbbb)]\n"
> > " .insert(ccccccccccccccccccccccc);");
> >
> > + verifyGoogleFormat(
> > + "aaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa)\n"
> > + " .aaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa)\n"
> > + " .aaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaa,\n"
> > + " aaaaaaaaaaaaaaaaaaa,\n"
> > + " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);");
> > +
> > // Here, it is not necessary to wrap at "." or "->".
> > verifyFormat("if
> (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaa) ||\n"
> > " aaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {\n}");
> >
> >
> > _______________________________________________
> > 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/20130219/a0a5f761/attachment.html>
More information about the cfe-commits
mailing list