r175432 - Prevent line breaks that make stuff hard to read.
Aaron Ballman
aaron at aaronballman.com
Mon Feb 18 17:30:35 PST 2013
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 --------------
A non-text attachment was scrubbed...
Name: Format.cpp.patch
Type: application/octet-stream
Size: 1032 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130218/213b950d/attachment.obj>
More information about the cfe-commits
mailing list